Add IOperation support for query operations. #21356
Conversation
Implements the proposal discussed in dotnet#17838 (comment). 1. We will have an `IQueryExpression` representing the topmost query expression. 2. This will point to the last query clause (`IQueryClause`) or continuation (`IQueryContinuation`) in the unrolled lowered bound tree - we are not got going to reverse the tree to match source. 3. `IQueryClause` will have a `QueryClauseKind` field indicating the type of query clause, which should match the syntax/language specification query clause/operator kinds.
|
FYI: There are still merge conflicts and I need to merge Heejae's parent pointer change into this branch, so the tests are expected to fail. I am sending out this PR early to start the review process. #Closed |
|
All merge conflicts have now been resolved. #Closed |
| return new LazyJoinIntoQueryClause(underlyingExpression, _semanticModel, syntax, type, constantValue); | ||
|
|
||
| default: | ||
| throw ExceptionUtilities.Unreachable; |
There was a problem hiding this comment.
throw UnexpectedValue(queryClauseKind) #Closed
|
|
||
| private IOperation CreateBoundQueryOrContinuationOrOrderExpressionOperation(BoundQueryClause boundQueryClause) | ||
| { | ||
| switch(boundQueryClause.Syntax.Kind()) |
There was a problem hiding this comment.
why does one use the syntax kind, and the other uses a different kind? #Closed
| return CreateBoundOrderingExpressionOperation(boundQueryClause, OrderKind.Descending); | ||
|
|
||
| case SyntaxKind.QueryBody: | ||
| return boundQueryClause.Value != null ? Create((BoundQueryClause)boundQueryClause.Value) : null; |
There was a problem hiding this comment.
how do you get nul lhere? #Closed
There was a problem hiding this comment.
This was just a defensive guard, I'll remove it unless any test breaks. #Closed
| return boundQueryClause.Value != null ? Create((BoundQueryClause)boundQueryClause.Value) : null; | ||
|
|
||
| default: | ||
| throw ExceptionUtilities.Unreachable; |
There was a problem hiding this comment.
throw UnexpecteValue #Closed
|
|
||
| private QueryClauseKind? GetQueryClauseKind(BoundQueryClause boundQueryClause) | ||
| { | ||
| switch(boundQueryClause.Syntax.Kind()) |
There was a problem hiding this comment.
you use map some syntax kins to query clause kinds here. why not map them all? #Closed
There was a problem hiding this comment.
Sure, I will pull all of the syntax kind comparisons into a single switch statement. #Closed
| /// <summary> | ||
| /// Underlying reduced expression for the query clause. This is normally the invocation expression for the underlying linq call. | ||
| /// </summary> | ||
| IOperation ReducedExpression { get; } |
There was a problem hiding this comment.
consider 'TranslatedExpression' #Closed
There was a problem hiding this comment.
Sure, will change. #Closed
| /// For example, for the query expression "from x in set where x.Name != null select x.Name", the select clause is the last clause of the unrolled query expression, | ||
| /// with the where clause as one of its descendant, and the from clause as the descendant of the where clause. | ||
| /// </summary> | ||
| IOperation LastClauseOrContinuation { get; } |
There was a problem hiding this comment.
you might want to give an example with a continuation. #Closed
| /// <summary> | ||
| /// Represents no ordering for an <see cref="IOrderingExpression"/>. | ||
| /// </summary> | ||
| None = 0x0, |
There was a problem hiding this comment.
is this possible? seems unlikely. if not, i would recommend removing enum and just having a bool "IsDescending" #Closed
There was a problem hiding this comment.
I have matched the API of other XXXKind enums for the IOperation APIs. Let me open a separate issue to tackle all the None kinds. #Closed
| Ascending = 0x1, | ||
|
|
||
| /// <summary> | ||
| /// Represents an ascending ordering for an <see cref="IOrderingExpression"/>. |
There was a problem hiding this comment.
'descending ordering' #Closed
| public enum QueryClauseKind | ||
| { | ||
| /// <summary> Indicates an invalid clause kind.</summary> | ||
| None = 0x00, |
There was a problem hiding this comment.
is this possible? #Closed
|
ping @dotnet/roslyn-compiler for review #Closed |
1 similar comment
|
ping @dotnet/roslyn-compiler for review #Closed |
| /// <summary>Indicates an <see cref="IQueryExpression"/>.</summary> | ||
| QueryExpression = 0x127, | ||
| /// <summary>Indicates an <see cref="IOrderingExpression"/> in an <see cref="IOrderByQueryClause"/>.</summary> | ||
| OrderingExpression = 0x128, |
There was a problem hiding this comment.
OrderingExpression [](start = 8, length = 18)
I am not sure why Ordering is singled out as an expression. What makes it special by comparison to other clauses? Can ordering be stand-alone, without query expression? #Closed
| @@ -216,5 +221,10 @@ public enum OperationKind | |||
|
|
|||
| /// <summary>Indicates an <see cref="IDefaultCaseClause"/>.</summary> | |||
| DefaultCaseClause = 0x412, | |||
|
|
|||
| /// <summary>Indicates an <see cref="IQueryClause"/>.</summary> | |||
| QueryClause = 0x413, | |||
There was a problem hiding this comment.
QueryClause = 0x413, [](start = 8, length = 20)
Is this kind ever used? #Closed
| /// This interface is reserved for implementation by its associated APIs. We reserve the right to | ||
| /// change it in the future. | ||
| /// </remarks> | ||
| public interface IQueryContinuation : IOperation |
There was a problem hiding this comment.
IQueryContinuation [](start = 21, length = 18)
Should we treat IQueryContinuation as just another query clause? Then we might be able to strongly type QueryBody as IQueryClause and probably can do the same for IQueryExpression.LastClauseOrContinuation. #Closed
| QueryClauseKind ClauseKind { get; } | ||
|
|
||
| /// <summary> | ||
| /// Underlying reduced expression for the query clause. This is normally the invocation expression for the underlying linq call. |
There was a problem hiding this comment.
Underlying reduced expression for the query clause. This is normally the invocation expression for the underlying linq call. [](start = 12, length = 124)
Perhaps it is worth mentioning that that call refers to other query clauses. #Closed
|
@mavasani It looks like there are some legitimate test failures. #Closed |
|
@AlekseyTs Following 2 tests are failing due to current GetOperation implementation: I expect them to pass as soon as @heejaechang PR for changes to GetOperation gets merged in. #Pending |
| PopulateRangeVariableMapForAnonymousType(node.Syntax, paramRef, nodeRangeVariables, firstUnmappedRangeVariable) | ||
| If parameter.Type.IsErrorType() Then | ||
| ' Skip adding to the range variable map for error case. | ||
| firstUnmappedRangeVariable += 1 |
There was a problem hiding this comment.
firstUnmappedRangeVariable += 1 [](start = 28, length = 31)
It doesn't look like it is safe to assume that only one range variable should be skipped in this case. Probably we should simply exit the loop. I'd like to take a closer look at the specific scenarios, for which we are taking this code path. #Closed
There was a problem hiding this comment.
As we discussed offline, I have changed the code to exit the function at this point and also removed the assert in the rewriter.
In reply to: 139533872 [](ancestors = 139533872)
| Dim pass2Rewriter As New QueryLambdaRewriterPass2 | ||
| Return pass2Rewriter.VisitLambda(rewrittenLambda) | ||
| Catch ex As CancelledByStackGuardException | ||
| Return node |
There was a problem hiding this comment.
Return node [](start = 16, length = 11)
Handling potential stack overflow in this manner can be a source of hard to repro bug reports. Let's open a follow up issue to confirm whether we want to use the guard (BoundTreeRewriterWithStackGuard) at all. #Closed
We are requesting IOperation for a single clause In reply to: 328924294 [](ancestors = 328924294,327889585) Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/QueryExpressions.vb:2181 in b63c6e6. [](commit_id = b63c6e6, deletion_comment = False) |
|
Done with review pass (iteration 8) #Closed |
The comment In reply to: 330358284 [](ancestors = 330358284,328924294,327889585) Refers to: src/Compilers/VisualBasic/Test/Semantic/Semantics/QueryExpressions.vb:2181 in b63c6e6. [](commit_id = b63c6e6, deletion_comment = False) |
|
@AlekseyTs Addressed your feedback. |
| Private ReadOnly _uniqueNodes As New HashSet(Of BoundParameter) | ||
|
|
||
| Public Overrides Function VisitParameter(node As BoundParameter) As BoundNode | ||
| node = DirectCast(MyBase.VisitParameter(node), BoundParameter) |
There was a problem hiding this comment.
Is the call to MyBase.VisitParameter() necessary?
There was a problem hiding this comment.
You are correct, I will remove it.
|
@dotnet-bot retest windows_debug_unit32_prtest please |
| Module Module1 | ||
| Sub Main() | ||
| Dim q As New QueryAble() | ||
| Dim q1 As Object = From s In q Where s > 0 |
| Module Module1 | ||
| Sub Main() | ||
| Dim q As New QueryAble2() | ||
| Dim q1 As Object = From s In q |
|
@dotnet-bot retest windows_release_unit64_prtest please |
|
@dotnet-bot retest windows_release_unit32_prtest please |
1 similar comment
|
@dotnet-bot retest windows_release_unit32_prtest please |
Implements the proposal discussed in #17838 (comment).
IQueryExpressionrepresenting the topmost query expression.IQueryClause) or continuation (IQueryContinuation) in the unrolled lowered bound tree - we are not got going to reverse the bound tree to match source.IQueryClausewill have aQueryClauseKindfield indicating the type of query clause, which should match the syntax/language specification query clause/operator kinds.