Skip to content

Commit

Permalink
Clean up features that can now just be assumed true (#2773)
Browse files Browse the repository at this point in the history
  • Loading branch information
jelopezf authored Dec 18, 2024
1 parent c3acfe9 commit cce6c4b
Show file tree
Hide file tree
Showing 5 changed files with 7 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -424,45 +424,19 @@ protected virtual bool IsValidAsyncOrImpureNode(TexlNode node, TexlBinding bindi
Contracts.AssertValue(binding);

var isAsync = binding.IsAsync(node);
var isPure = binding.IsPure(node);

if (!isAsync && isPure)
{
return true;
}

// Async predicates and impure nodes are not supported unless Features say otherwise.
// Let CallNodes for delegatable async functions be marked as being Valid to allow
// expressions with delegatable async function calls to be delegated

// Impure nodes should only be marked valid when Feature is enabled.
if (!isPure && !binding.Features.AllowImpureNodeDelegation)
if (!isAsync)
{
TrackingProvider.Instance.SetDelegationTrackerStatus(DelegationStatus.ImpureNode, node, binding, trackingFunction ?? Function, DelegationTelemetryInfo.CreateImpureNodeTelemetryInfo(node, binding));
return true;
}
else
{
if (!isAsync)
{
return true;
}
else if (binding.Features.AllowAsyncDelegation)
{
// If the feature is enabled, enable delegation for
// async call, first name and dotted name nodes.
return (node is CallNode) || (node is FirstNameNode) || (node is DottedNameNode);
}
}

if (isAsync)
{
TrackingProvider.Instance.SetDelegationTrackerStatus(DelegationStatus.AsyncPredicate, node, binding, trackingFunction ?? Function, DelegationTelemetryInfo.CreateAsyncNodeTelemetryInfo(node, binding));
// Enable delegation for async call, first name, and dotted name nodes.
return (node is CallNode) || (node is FirstNameNode) || (node is DottedNameNode);
}

var telemetryMessage = string.Format(CultureInfo.InvariantCulture, "Kind:{0}, isAsync:{1}, isPure:{2}", node.Kind, isAsync, isPure);
SuggestDelegationHintAndAddTelemetryMessage(node, binding, telemetryMessage);

return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public override bool IsOpSupportedByTable(OperationCapabilityMetadata metadata,
/* Left node can be first name, row scope lambda or a lookup column */
(_binaryOpNode.Left.Kind == NodeKind.FirstName || binding.IsFullRecordRowScopeAccess(_binaryOpNode.Left) || (_binaryOpNode.Left.Kind == NodeKind.DottedName && binding.GetType((_binaryOpNode.Left as DottedNameNode).Left).HasExpandInfo)) &&
/* Right has to be a single column table */
((_binaryOpNode.Right.Kind == NodeKind.Table || binding.GetType(_binaryOpNode.Right)?.IsColumn == true) && (binding.Features.AllowAsyncDelegation || !binding.IsAsync(_binaryOpNode.Right)));
(_binaryOpNode.Right.Kind == NodeKind.Table || binding.GetType(_binaryOpNode.Right)?.IsColumn == true);

if (!(isRHSFirstName || isRHSRecordScope || isCdsInTableDelegation))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,12 @@ private bool IsSupportedNode(TexlNode node, OperationCapabilityMetadata metadata
// Filter(Accounts, 'Account Name' in ["Foo", Bar"]) - Direct table use
// Set(Names, ["Foo", Bar"]); Filter(Accounts, 'Account Name' in Names) - Using variable of type table
// ClearCollect(Names, Accounts); Filter(Accounts, 'Account Name' in Names.'Account Name') - using column from collection.
// This won't be delegated if the AllowAsyncDelegation- Filter(Accounts, 'Account Name' in Accounts.'Account Name') as Accounts.'Account Name' is async.
// Filter(Accounts, 'Account Name' in Accounts.'Account Name') as Accounts.'Account Name' is async.
if (isRHSNode
&& opDelStrategy is BinaryOpDelegationStrategy { Op: BinaryOp.In }
&& !binding.IsRowScope(node)
&& binding.GetType(node).IsTable
&& binding.GetType(node).IsColumn
&& (binding.Features.AllowAsyncDelegation || !binding.IsAsync(node))
&& opDelStrategy.IsOpSupportedByTable(metadata, node, binding))
{
return true;
Expand Down
12 changes: 0 additions & 12 deletions src/libraries/Microsoft.PowerFx.Core/Public/Config/Features.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,6 @@ public sealed class Features
/// </summary>
internal bool RestrictedIsEmptyArguments { get; init; }

/// <summary>
/// Allow delegation for async calls (delegate using awaited call result).
/// </summary>
internal bool AllowAsyncDelegation { get; init; }

/// <summary>
/// Allow delegation for impure nodes.
/// </summary>
internal bool AllowImpureNodeDelegation { get; init; }

/// <summary>
/// Updates the FirstN/LastN functions to require a second argument, instead of
/// defaulting to 1.
Expand Down Expand Up @@ -131,8 +121,6 @@ internal Features(Features other)
PowerFxV1CompatibilityRules = other.PowerFxV1CompatibilityRules;
PrimaryOutputPropertyCoercionDeprecated = other.PrimaryOutputPropertyCoercionDeprecated;
IsUserDefinedTypesEnabled = other.IsUserDefinedTypesEnabled;
AllowImpureNodeDelegation = other.AllowImpureNodeDelegation;
AllowAsyncDelegation = other.AllowAsyncDelegation;
AsTypeLegacyCheck = other.AsTypeLegacyCheck;
JsonFunctionAcceptsLazyTypes = other.JsonFunctionAcceptsLazyTypes;
IsLookUpReductionDelegationEnabled = other.IsLookUpReductionDelegationEnabled;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,7 @@ private bool TryUpdateFeatures(string featureName, bool featureValue)
{
switch (featureName)
{
// When we move to C# 12 we can use nameof(Features.AllowAsyncDelegation):
case "AllowAsyncDelegation":
this.Features = new Features(this.Features) { AllowAsyncDelegation = featureValue };
return true;
case "AllowImpureNodeDelegation":
this.Features = new Features(this.Features) { AllowImpureNodeDelegation = featureValue };
return true;
// When we move to C# 12 we can use nameof(Features.AsTypeLegacyCheck):
case "AsTypeLegacyCheck":
this.Features = new Features(this.Features) { AsTypeLegacyCheck = featureValue };
return true;
Expand Down

0 comments on commit cce6c4b

Please sign in to comment.