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

EnC: Check member modifier updates in semantic analysis #54406

Merged
merged 4 commits into from
Jun 29, 2021

Conversation

tmat
Copy link
Member

@tmat tmat commented Jun 26, 2021

Analyzing modifier changes in semantic analysis has multiple advantages:

  1. We can detect the actual impact of change on the generated code. In cases when changing a modifier does not affect the generated code we don't need to issue a rude edit.
  2. When analyzing syntax of a type or its member without semantic information we do not know whether to report a rude edit or not - the type may be reloadable, in which case most rude edits are not applicable.

The PR also fixes a few issues uncovered by the changes in the analyzer.

@tmat
Copy link
Member Author

tmat commented Jun 26, 2021

@davidwengier

@tmat
Copy link
Member Author

tmat commented Jun 26, 2021

@davidwengier Seems like we are missing tests that update attributes on non-type members (specifically events).

@davidwengier
Copy link
Contributor

Seems like we are missing tests that update attributes on non-type members (specifically events).

Thanks. Logged #54430

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

The actual modifier update bits looks good. The analyzer changes are difficult to reason about without debugging through, but I trust the tests are doing the right thing, so I'm sure it's fine. Plus you're fairly trustworthy too 😛

AnalyzeCustomAttributes(oldSymbol, newSymbol, capabilities, diagnostics, semanticEdits, syntaxMap, cancellationToken);
if (isDeclarationWithInitializer)
{
AnalyzeSymbolUpdate(oldSymbol, newSymbol, newDeclaration, capabilities, diagnostics, semanticEdits, syntaxMap, cancellationToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure this is covered by some other case (and hopefully tests), but this looks like an issue. Whether there is an initializer, we still need to report rude edits for attributes etc., right?

Copy link
Member Author

@tmat tmat Jun 28, 2021

Choose a reason for hiding this comment

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

We call DeferConstructorEdit for constructors that emit member initializers and declarations with initializers. This takes care of handling symbol changes (attributes, modifiers) for the latter. The former is handled in AddConstructorEdits where we actually issue edits for the constructor.

@tmat tmat marked this pull request as ready for review June 29, 2021 19:41
@tmat tmat requested a review from a team as a code owner June 29, 2021 19:41
@tmat tmat merged commit 09512f4 into dotnet:main Jun 29, 2021
@ghost ghost added this to the Next milestone Jun 29, 2021
@tmat tmat deleted the RudeEditsSemantics branch June 29, 2021 21:16
@tmat tmat modified the milestones: Next, 17.0.P2 Jun 29, 2021
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.

3 participants