Implement field null resilience analysis #77127
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Area-Compilers
untriaged
RikkiGibson
changed the title
cston
reviewed
Mar 4, 2025
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedBackingFieldSymbol.cs
Show resolved
Hide resolved
cston
reviewed
Mar 4, 2025
cston
reviewed
Mar 4, 2025
cston
reviewed
Mar 4, 2025
cston
reviewed
Mar 4, 2025
cston
approved these changes
Mar 7, 2025
333fred
approved these changes
Mar 10, 2025
|
I have updated the speclet links, to refer to the exact version of the spec which this PR is implementing. There has been an additional suggestion in dotnet/csharplang#9184, to instead make it so the backing field is always nullable a la I started implementing that approach, and I think it is promising, but, it is proving to be a lot of churn, especially since backing fields are all over the place in our tests. The number of increased attributes etc. is something that may need more scrutiny. So, I would like to start by merging this PR as-is, and sending a follow up depending on how the next step of the work goes. I consider this approach to be essentially compatible with the existing one in this PR--the "always annotate" approach, if anything, being more lax and warning in fewer situations. Please let me know if there are any objections/concerns. Thanks. |
This was referenced Mar 14, 2025
333fred
added a commit
that referenced
this pull request
Mar 20, 2025
* wip
* wip
* wip
* wip
* wip
* wip
* wip
* remove some commented out code
* still cleaning up
* revert file
* clean up + option
* wip
* dispose of disable of intellicode line completions
* comments
* comments
* add quota exceeded checks
* fix tests
* revert file
* remove unused optional params
* last few fixes
* feedback
* fix tests
* small fix
* fix
* feedback
* feedback
* removed per comments
* remove unused usings
* Fixer/Analyzer for implementing method
TODO
- Try invoking copilot service from Features layer (needs GenerateMethod APIs added to EA)
- Try invoking semantic search from Features layer
Reviews done:
- Skimmed through the proposal edit approach used for ///
- Excluded diff for ///, does not exist in copilot service
* feedback
* feedback
* move stuff around with new EA structure in roslyn
* wip
* feedback
* feedbacl
* fix
* feedback
* feedback
* feedback
* revert
* remove null check
* clean up
* fix
* fix tests
* local ad ons
* local ad ons
* Add main bits for generating method implementation
* todo next
* update APIs
* trial 1
* Fix compile after merge
* temp1
* temp2
* Implement method, end-to-end for one-off code action
* corrections
* cleanup
* cleanup preliminary check
* Fix bug
* cleanup and fixes
* remove local changes for pr
* correct spacing
* cleanup
* Add unit test - takes code block suggestion to replace with method body
* Add test, support expression body clause later
* Change the FAR API call
* using `.Parent.FirstAncestorOrSelf`
* Improve wrapper implementation
* Apply first round code review
* Merge DualChangeAction into DocumentChangeAction
* Make IDE3000 not configurable
This diagnostic is always hidden (no UI) for the sole support of the
associated code fix provider.
* Simplify analyzer code and improve robustness
* Simplifies fixer and applies most remaining feedback
* Fix compile related to DocumentChangeAction
* Cleanup messaging
* Add resources for messaging
* Adds a few basic tests
* Undo most recent analyzer changes,
to keep the throw statement/expression check
- Adds more test
- Removes dead code
- [ ] Cleanup more code
- [ ] Fix failing tests
- [ ] Rename API make consistent
- [ ] Revisist triple slash docs
- [ ] Change SyntaxNode to MemberDeclaration
- [ ] Remove unexpected null checks
* Apply PR feedback
- [x] Adds more test
- [x] Removes dead code
- [x] Cleans up more code
- [ ] Fix failing tests
- [x] Renames API to make consistent
- [x] Revisits triple slash docs
- [x] API Accepts SyntaxNode
- [x] Removes unreachable code
* Tests renamed
* Correct test setup
- and do minor Cleanup
* Improves readability and removes unreachable code
* Correct mistakes in tests
* Adds more unit tests
* Fix formatting (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0055)
* Fixes Analyzer tests
- [ ] Fixer seems not activates in tests
* Makes sure original and replacement text are properly aligned
* Apply partial PR feedback
- Test correction on placement of diagnostic span
* cleanup
* Applies feedback to change API surface
* All tests pass
* tests are actually failing
* Fix Quota bug and show failing tests
* Update tests for error conditions where diagnostics are not corrected
* undo IsImplementNotImplementedExceptionEnabledAsync change
* Fixes remaining tests
* cleanup
* Add back referencedSymbols to API call to help with perf
* Gracefully comment when result is unexpected
* Remove unused resource
* Simplify error messages
* Cleanup tests
* Add missing entry for IDE3000
* Add Missing Help Link
* mid stage
* Removes QuotaExceeds property, received as message
* Update signature
* Fix missed out warning
* Fix test correctness issue picked up by CI
* Add logging
* Apply PR feedback
* Fix run code analysis on solution not reporting results
Fixes #77495
* Use HangMitigatingTimeout for cases where operations do not complete synchronously
* some PR feedback
* spacing
* Move test
* Apply most remaining feedback
* Applies feedback
* Add runtime async to official build (#77537)
* Signature now strictly uses MemberDeclarationSyntax
* Update message for analyzer referencing newer compiler than host (#77541)
* Updates tests
* field implementation is not supported as method or property
see test below
```
[InlineData("int myField;", typeof(FieldDeclarationSyntax))]
public async Task TestInvalidNodeReplacement(string syntax, Type type)
```
* Fix NFW due to invoking Workspace.RaiseEventForHandlers in the CA process (#77546)
NFW telemetry indicates Solution.Workspace is getting called in our server process. This is due to a recent change I made around immediate eventing. In the server process, there are no immediate (or standard) workspace event handlers, but due to this recent changewe were always calling RaiseEventForHandlers for the immediate handlers without checking for their presence.
* Implement field null resilience analysis (#77127)
Co-authored-by: Fred Silberberg <[email protected]>
* Fix incremental generator in deterministic key file (#77553)
* Apply PR feedback
* Disable downloading the runtime packages during restore and instead ensure that it happens when needed (pack) (#77515)
During the most recent SDK insertion, allocation regressions were reported in the C# editing speedometer test. Investigation yielded that these were because of restore failures in the test project as the runtime packages are not published at the point of the SDK insertion.
Looking at the MS.CodeAnalysis.LanguageServer project closer revealed that these runtime identifiers needed to only be set during the pack task. This PR moves setting the RuntimeIdentifiers property to only be done during the pack task, and thus the restore task won't attempt to download these packages. As the test doesn't perform a pack on the roslyn solution, this should prevent future SDK insertions from hitting this issue again.
* Simplifies assumptions - Only trigger fixer when service available
* nit spacing
* Fix preview window for 'implement NotImplementedException'
* Simmplify
* Remove uneeded helper
* Fix Stack Trace Explorer for additional documents (#77517)
Update IDocumentNavigationService implementation in VS to work on any TextDocument
Fix IDocumentNavigationService extension to use TextDocument
Fix Stack Trace Explorer to search for AdditionalDocuments when trying to find a match in file path or name
Partial fix for #77499
There might be more cases where finding symbols doesn't work but filepath+line navigation should now work
* Update dependencies from https://github.com/dotnet/arcade build 20250311.4 (#77569)
Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks
From Version 9.0.0-beta.25111.5 -> To Version 9.0.0-beta.25161.4
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* Update PublishData.json (#77589)
* Update main-merge.yml to not run on forks
* Fix typo (#77595)
* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250313.4 (#77604)
Microsoft.SourceBuild.Intermediate.source-build-reference-packages
From Version 10.0.616001 -> To Version 10.0.616304
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* Filter down the list of files we need to examing when looking for a :base(...) call in find refs
* Move feature-oriented helpers to appropriate location
* Ensure we don't touch Solution.Workspace if we don't have to
This is a bit more of a defense-in-depth fix to fix a potential NFW.
* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250314.1 (#77623)
Microsoft.SourceBuild.Intermediate.source-build-reference-packages
From Version 10.0.616304 -> To Version 10.0.616401
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* Update dependencies from https://github.com/dotnet/arcade build 20250314.2 (#77624)
Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks
From Version 9.0.0-beta.25161.4 -> To Version 9.0.0-beta.25164.2
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* Correct how we pick up options for `ICopilotOptionsService` (#77620)
* Corrections to how we pick up roslyn options settings
* Cleanup
* Set default for Implement with Copilot to be false
* update for copilot.general.editor.enableGenerateDocumentationComment
* Update BuildActionTelemetryTable tool
* Add feature flag controlling Copilot prompt in Semantic Search (#77562)
* Fix BuildActionTelemetryTable project
* Update CodeActionDescriptionsMap.
Updates program to generate new description map when analyzers are added.
* Fix typo
* Remove dead code, use generated regex, use filescoped namespace.
* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250317.1 (#77649)
Microsoft.SourceBuild.Intermediate.source-build-reference-packages
From Version 10.0.616401 -> To Version 10.0.616701
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* On-the-fly-docs Pass along additional context (#77510)
* wip
* works end to end
* some feedback
* feedback
* fix bugs
* tests
* revert settings
* fix code analysis service
* fix formatting
* Feedback
* Change vs deps flow (#77651)
* Fix generation of attribute with array constant
* Make ISemanticSearchCopilotUIProvider import lazy to avoid loading VS.EA.Copilot (#77516)
* Fix watch window completion window upon manual completion invocation (#77656)
* Fix watch window completion window upon manual completion invocation
Addresses https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2287966
This recently regressed due to this recent PR: #77204
The issue here is that I changed the csharp debugger context's projection buffer creation code from always having a one char string as the second sourceSpan to sometimes having that string be empty. This allowed the completion context to include items for which a semicolon would close that scope. However, by having a zero length span, a seam was created for a tracking span at priorTrackingSpan to be introduced into the set of tracking positions that can contribute to completion. Editor completion doesn't handle this well, so we're best off always sending a non-zero length source Span, so we do so and just use a space as the value.
* Generate Documentation - Bug Fixes (#77641)
* fix some bugs
* Feedback
* Fix reflection
* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250318.1 (#77672)
Microsoft.SourceBuild.Intermediate.source-build-reference-packages
From Version 10.0.616701 -> To Version 10.0.616801
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* Fix typo (#77678)
disalloed => disallowed.
* Fix SkipApplyOptimizationData parameter (#77677)
* Support textDocument/semanticTokens/full
* Cleanup and make semantic token processing and testing code more consistent
* Fix test
* Add CI validation of Semantic Search API lists (#77535)
* raw strings
* Fix bug where exact path match would throw for additional files (#77583)
From /pull/77517/files/45c0e103f76f36bed6004f836d3dcfeae4bfae0d#r1992506030
* Update CSharpCopilotCodeAnalysisService.cs (#77691)
* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250319.2 (#77694)
Microsoft.SourceBuild.Intermediate.source-build-reference-packages
From Version 10.0.616801 -> To Version 10.0.616902
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
---------
Co-authored-by: Ankita Khera <[email protected]>
Co-authored-by: Maryam Ariyan <[email protected]>
Co-authored-by: Sam Harwell <[email protected]>
Co-authored-by: Rikki Gibson <[email protected]>
Co-authored-by: Todd Grunke <[email protected]>
Co-authored-by: Jared Parsons <[email protected]>
Co-authored-by: Cyrus Najmabadi <[email protected]>
Co-authored-by: Andrew Hall <[email protected]>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Jan Jones <[email protected]>
Co-authored-by: Arun Chander <[email protected]>
Co-authored-by: Kauwai Lucchesi <[email protected]>
Co-authored-by: Jason Malinowski <[email protected]>
Co-authored-by: Tomáš Matoušek <[email protected]>
Co-authored-by: Joey Robichaud <[email protected]>
Co-authored-by: Ankita Khera <[email protected]>
Co-authored-by: David Barbet <[email protected]>
Co-authored-by: Joey Robichaud <[email protected]>
Co-authored-by: David Wengier <[email protected]>
Co-authored-by: Bill Wagner <[email protected]>
Co-authored-by: PaddiM8 <[email protected]>
2 tasks
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #76662
See also dotnet/csharplang#9184
Mainly what I would like feedback on here is: does the way the API works here make sense? We basically have a side API
SynthesizedBackingFieldSymbol.GetInferredNullableAnnotation(), which at this point we take care to call only from the NullableWalker.Since calling this API results in binding and flow analysis, I am concerned about re-entrancy. What if doing this flow analysis, causes us to ask about the field's nullable annotation, which causes us to flow analyze, etc... Hence the current limited implementation.
But I am also concerned about coherence of the public APIs if we do not expose this information in a broader/more holistic way. For example, if we hover on
fieldinpublic string Prop => field ??= "a";, and it saysstring Prop.field, it is confusing, because we in fact inferredstring?for it. So my question is essentially how can we surface this inferred nullability in a way which reduces the risk of stack cycles.