Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: IOperation support for queries #17838

Closed
bkoelman opened this issue Mar 14, 2017 · 21 comments
Closed

Feature request: IOperation support for queries #17838

bkoelman opened this issue Mar 14, 2017 · 21 comments
Labels
Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation Feature Request Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@bkoelman
Copy link
Contributor

bkoelman commented Mar 14, 2017

Feeding the next code sample to an OperationWalker hits nothing. Please consider IOperation support for queries.

var query =
    from ch in text
    where ch >= 'a' && ch <= 'z'
    select char.ToUpperInvariant(ch);
@jinujoseph
Copy link
Contributor

cc/ @genlu

@jinujoseph jinujoseph added this to the 15.3 milestone Mar 14, 2017
@genlu
Copy link
Member

genlu commented Mar 14, 2017

LINQ was left out with the intention to support them in future, when we made the effort to release IOperation last year. We need to discuss again whether it should be in V1 or not.

@jinujoseph jinujoseph modified the milestones: 15.3, 15.later Apr 18, 2017
@mavasani
Copy link
Contributor

mavasani commented Apr 20, 2017

Note that not implementing this completely means that our current implementation makes no analyzer callbacks for anything inside a query - parameter/field/local references, invocations, etc.

I don't think that is acceptable for any new language feature. We need to take one of the following approaches:

  1. Ensure that we have a temporary IOperationWithChildren type that ensures the bound child nodes of new language features to still be part of the Operation tree, so the analyzer driver can make callbacks for them
  2. Design an API for the language feature.

@mavasani mavasani modified the milestones: 15.3, 15.later Apr 20, 2017
@mavasani
Copy link
Contributor

I am moving this back to 15.3 to discuss in the API review meeting.

@mavasani
Copy link
Contributor

I have opened #18968 to track the work to add IOperationWithChildren internal type as a workaround for NYI features.

@jinujoseph
Copy link
Contributor

Design Meeting Decision

For now lets unblock users with the workaround defined in #18968 . we need to come back and discuss about what the API for queries looks like.

@jinujoseph jinujoseph modified the milestones: 15.later, 15.3 Apr 27, 2017
@jinujoseph jinujoseph assigned 333fred and unassigned mavasani May 2, 2017
@jinujoseph
Copy link
Contributor

we need to collect a lot of information here so that we can then get together to discuss how we want this API to look like.

We need to identify differences between the languages (for example VB is semantic and C# is syntactic). We need to enumerate all the different types of queries. We need to understand all the strange VB cases. We need to get info on transparent identifiers and the like.

Basically, you needs to go read up a lot on these guys and collect a lot of info as we'll likely need a lot of design around this.

@jinujoseph jinujoseph assigned mavasani and unassigned 333fred May 3, 2017
@jinujoseph jinujoseph modified the milestones: 15.5, 15.later Jul 3, 2017
mavasani added a commit to mavasani/roslyn that referenced this issue Aug 8, 2017
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.
@mavasani
Copy link
Contributor

See #21356 (comment) for discussion

@CyrusNajmabadi
Copy link
Member

Design decisions:
Vastly reduce the IQueryExpression API. Do not expose clauses. Do not find a way to map from a query clause node to anything. Rename IQueryExpression to something like ITranslatedQueryExpression (name TBD).

Figure out engineering way to "lower" the current VB query tree so that query lambdas are rewritten to regular lambdas. This will ensure that range variables are rewritten into lambda parameter access. C# and VB trees should look more similar.

--

In future (potentially based on customer demand) we can supply a higher level Query API that more closely matches the original source (along with things like range variables and the like).

@tmat tmat removed the Urgency-Soon label Sep 6, 2017
@jinujoseph jinujoseph added Urgency-Now 4 - In Review A fix for the issue is submitted for review. labels Sep 14, 2017
@mavasani
Copy link
Contributor

Removing the Urgency-Now label and marking as 15.later - we have basic support for translated query expressions now, which matches our v1 scope. Keeping this issue open to track richer query APIs for v2.

@mavasani mavasani modified the milestones: 15.5, 15.later Sep 20, 2017
@mavasani mavasani removed their assignment Sep 20, 2017
@jinujoseph jinujoseph modified the milestones: 15.6, Unknown Nov 2, 2017
@jinujoseph jinujoseph added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Jun 23, 2018
@mavasani
Copy link
Contributor

#28098 tracks exposing richer IOperation API for queries. This issue was closed as we do expose an ITranslatedQueryOperation for queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation Feature Request Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

No branches or pull requests

7 participants