Skip to content

Expose ParseOptions on generator context#46919

Merged
chsienki merged 2 commits intodotnet:masterfrom
chsienki:sg_parse_options
Aug 26, 2020
Merged

Expose ParseOptions on generator context#46919
chsienki merged 2 commits intodotnet:masterfrom
chsienki:sg_parse_options

Conversation

@chsienki
Copy link
Copy Markdown
Member

@chsienki chsienki commented Aug 18, 2020

Small PR that exposes the parse options that will be used to the consuming generator

@chsienki chsienki marked this pull request as ready for review August 18, 2020 20:08
@chsienki chsienki requested a review from a team as a code owner August 18, 2020 20:08
@chsienki
Copy link
Copy Markdown
Member Author

@dotnet/roslyn-compiler for review please

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests for the added public API?

Also, just for my context, can you clarify where those parse options come from (how are they computed)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test. The options are passed to the generator driver at construction time. In CSC we pass them here:

compilation = RunGenerators(compilation, Arguments.ParseOptions, generators, analyzerConfigProvider, additionalTextFiles, diagnostics);

Which come directly from the command line args.

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with review (iteration 1). Needs tests

@jcouv jcouv self-assigned this Aug 18, 2020
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 2)

@chsienki
Copy link
Copy Markdown
Member Author

Ping @dotnet/roslyn-compiler for a second review please :)

@333fred
Copy link
Copy Markdown
Member

333fred commented Aug 20, 2020

I'm not sure why this is necessary: if an analyzer author cares about the parse options, they have access to the compilation. They can either get a ParseOptions off one of the trees, or more likely, answer any questions by calling methods on the Compilation directly.

@chsienki
Copy link
Copy Markdown
Member Author

chsienki commented Aug 20, 2020

@333fred suggested we just add the ParseOptions to the compilation directly.

My thought is that generally on a compilation they aren't that interesting, as you can just ask questions of the compilation about 'what was parsed'. With generators though the parse options become more interesting, because it's saying 'how is the compiler going to parse this text I give it'.

We could move it to the compilation, but given the above, I don't see that it's particularly useful anywhere other than in a generator, so I think we should just keep the option on the context. (You can also always just go and get the options via the trees if you need to. This PR just makes it simpler/more obvious for generators to do so).

@chsienki chsienki merged commit 7843fd6 into dotnet:master Aug 26, 2020
@ghost ghost added this to the Next milestone Aug 26, 2020
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Hrmm... can you clarify how this would work? ParseOptions are specific to a tree, not the full compilation. So how can you expose something at teh compilation level?

333fred added a commit to 333fred/roslyn that referenced this pull request Aug 27, 2020
…-only-errors

* upstream/master: (236 commits)
  Fix bug when "End statement" is used in single-line if (dotnet#47062)
  Solution asset cache refactoring (dotnet#46948)
  add specific tests to validate behavior between keys and snapshots
  Extract into separate files
  rename parameters
  rename parameters
  rename parameters
  rename parameters
  Add CancellationToken parameters to SyntaxTreeOptionsProvider
  Reuse nullable override checks for delegate conversions (dotnet#46953)
  Introduce warning for multiple entry points (sync + async) (dotnet#46832)
  Switch from throwing NotImplementedException and return E_NOTIMPL
  Delete Building for Core CLR.md (dotnet#47146)
  Adjust PrintMembers to avoid boxing and avoid extra space (dotnet#47095)
  Track asynchronous operation in InProcLanguageServer
  Use Task.FromCanceled where appropriate
  Apply suggestions from code review
  Address feedback
  Expose ParseOptions on generator context (dotnet#46919)
  Remove redundant statement in added tests
  ...
@allisonchou allisonchou modified the milestones: Next, 16.8.P3 Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants