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.

Sign up for GitHub

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

Jump to bottom

Issue in analyzer callbacks for nested actions in symbol start analyzers #68484

Closed
mavasani opened this issue Jun 8, 2023 · 0 comments · Fixed by #68490
Closed

Issue in analyzer callbacks for nested actions in symbol start analyzers #68484

mavasani opened this issue Jun 8, 2023 · 0 comments · Fixed by #68490
Assignees
@mavasani
Labels
Area-Analyzers Bug Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner. untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@mavasani
Copy link
Contributor

mavasani commented Jun 8, 2023

Issue found by the unit test skipped in 2e0f6b9.

Core issue is as follows:

  1. Create a symbol start analyzer that registers nested operation actions for code within named types
  2. Register symbol end action for the outer type, but not for the nested type
  3. Race condition: Symbol end action callback for the outer type gets invoked before the nested operation callback for executable code within the nested type.

Note that the above does not repro if the analyzer registers a symbol end action also for the nested type.
@mavasani mavasani added Bug Area-Analyzers Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner. labels Jun 8, 2023
@mavasani mavasani self-assigned this Jun 8, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 8, 2023
mavasani added a commit to mavasani/roslyn that referenced this issue Jun 8, 2023
@mavasani
Analyzer workaround for dotnet#68484
5bea3c0
@mavasani mavasani mentioned this issue Jun 8, 2023
Closed
mavasani added a commit to mavasani/roslyn that referenced this issue Jun 8, 2023
@mavasani
Fixes dotnet#68484
d352b44 
Race condition is detailed in the description of dotnet#68484. Fix is to ensure that we also mark the members of nested types as dependent symbols so that the SymbolEnd actions for the outer type are executed only after callbacks for nested type members have been done.

Testing: This bug was identified by the newly added CSharpUsePrimaryConstructorDiagnosticAnalyzer. There is an existing unit test that failed intermittently due to this race condition: https://github.com/CyrusNajmabadi/roslyn/blob/f3e4a27818ea2495606f12fcabd986238b66efe4/src/Analyzers/CSharp/Tests/UsePrimaryConstructor/UsePrimaryConstructorTests.cs#LL741C27-L741C62
A workaround was added to the analyzer to register a dummy SymbolEnd action for nested types to work around this bug. This PR reverts that workaround and I verified that the test passes successfully for hundreds of iterations. I also attempted to create a standalone compiler unit test, but was unable to get this race condition to repro for the test even without the product change.
@mavasani mavasani mentioned this issue Jun 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mavasani mavasani

Labels
Area-Analyzers Bug Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
No milestone
2 participants
@mavasani and others