Fix crash in backing field nullability cycle scenario#77993
Merged
RikkiGibson merged 2 commits intodotnet:mainfrom Apr 8, 2025
Merged
Fix crash in backing field nullability cycle scenario#77993RikkiGibson merged 2 commits intodotnet:mainfrom
RikkiGibson merged 2 commits intodotnet:mainfrom
Conversation
ghost
added
Area-Compilers
untriaged
cston
reviewed
Apr 4, 2025
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/77991")] | ||
| public void Nullable_OrdinaryMethod_FieldStateAffectedByAssigningStruct() |
Contributor
Member
Author
There was a problem hiding this comment.
No, they are not. These are here to illustrate a more general problem with nullable state management in structs interacting with field-backed prop slot sharing.
cston
reviewed
Apr 4, 2025
| var comp = CreateCompilation(source); | ||
| comp.VerifyEmitDiagnostics( | ||
| // (5,5): error CS0200: Property or indexer 'S.Resilient' cannot be assigned to -- it is read only | ||
| // Resilient = "a", |
Contributor
cston
reviewed
Apr 4, 2025
|
|
||
| public S(bool ignored) : this() | ||
| { | ||
| Resilient.ToString(); // 5 |
Contributor
Member
Author
There was a problem hiding this comment.
This is the main scenario in bug #77991. The main reason is that the code in GetOrCreateSlot is using the field to "own" the shared slot in the context of the constructor. The field is therefore also used to give the slot its initial flow state (GetDefaultState()). That results in maybe-null state for the resilient field as a maybe-null annotation was inferred for it. And a not-null state for the other field since not-null was inferred for it.
Member
Author
There was a problem hiding this comment.
I think it will be good to come back and iron this out more completely for a future release, but for now, to just plug the gap which is resulting in a crash, and handle other possible cycles better at runtime.
Contributor
There was a problem hiding this comment.
Sounds good. Do we need to log an issue to follow up in a future release?
Member
Author
There was a problem hiding this comment.
No I think #77991 is adequate as a catch all, I'll ensure the tests with unexpected/missing warnings are all marked.
cston
reviewed
Apr 4, 2025
| public S((bool, bool) ignored2) | ||
| { | ||
| this = new(); | ||
| Resilient.ToString(); // 6 |
Contributor
Member
Author
There was a problem hiding this comment.
Same reason as above.
Member
|
[like] Julien Couvreur reacted to your message:
________________________________
From: Charles Stoner ***@***.***>
Sent: Friday, April 4, 2025 6:33:28 AM
To: dotnet/roslyn ***@***.***>
Cc: Julien Couvreur ***@***.***>; Review requested ***@***.***>
Subject: Re: [dotnet/roslyn] Fix crash in backing field nullability cycle scenario (PR #77993)
@cston commented on this pull request.
________________________________
In src/Compilers/CSharp/Test/Emit3/FieldKeywordTests.cs<#77993 (comment)>:
+ public S()
+ {
+ Resilient.ToString(); // 3
+ NonResilient.ToString(); // 4
+ }
+
+ public S(bool ignored) : this()
+ {
+ Resilient.ToString(); // 5
+ NonResilient.ToString();
+ }
+
+ public S((bool, bool) ignored2)
+ {
+ this = new();
+ Resilient.ToString(); // 6
Resilient.ToString(); // 6 <http://example.com/codeflow?start=24&length=26>
Why are we reporting a warning here, rather than initializing the state based on this = new()?
—
Reply to this email directly, view it on GitHub<#77993 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC7DQOO2MVOZOE4MMQUF5ZL2XYRTRAVCNFSM6AAAAAB2NLZ242VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDONBRHEYDIMBRGA>.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
cston
approved these changes
Apr 4, 2025
Member
Author
|
@dotnet/roslyn-compiler for a second review |
cston
approved these changes
Apr 4, 2025
Member
Author
|
@dotnet/roslyn-compiler for a second review. This is something we likely want to take thru QB for 17.14. |
jjonescz
approved these changes
Apr 8, 2025
Member
Author
|
/backport to release/dev17.14 |
Contributor
|
Started backporting to release/dev17.14: https://github.com/dotnet/roslyn/actions/runs/14338769927 |
Contributor
|
@RikkiGibson an error occurred while backporting to "release/dev17.14", please check the run log for details! Error: @RikkiGibson is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=RikkiGibson |
Member
Author
|
/backport to release/dev17.14 |
Contributor
|
Started backporting to release/dev17.14: https://github.com/dotnet/roslyn/actions/runs/14339040411 |
2 tasks
333fred
added a commit
that referenced
this pull request
Apr 12, 2025
* Addressing feedback * Addressing PR feedback * Addressing PR comments * Remove workaround for dotnet/msbuild#10306 The VS SDK was fixed, so the problem shouldn't occur anymore. * move to a workspace factory * No need for solution * Simplify handlers * Add helper * move more over * move more over * Add remove calls * Move handlers over * Move remote service over * async lock * async lock * async lock * More work * Fixup * Correct * Move to immutable dictionary * in progress * in progress * in progress * in progress * in progress * in progress * Simplify * Serialize * simplify * simplify * simplify * in progress * in progress * Reset code * Clelar * renames * Simplify * rename * No more null * Simplify * Exception throwing * Extensions: update feature status (#77923) * Add ignored directives feature status (#77965) * Extensions: misc checks on receiver parameter and extension members (#77937) * Add porject back in * CHange how locking works * Simplify * Docs * Add new message for getting registered method names * IN progres * in progress * Simplify * Simplify * Handle exceptions * Simplify * Docs * move method * Simplify and regions * Tweaks * Docs * Break into files * Simplify exception handling * Simplify * Simplify * Move * Docs * Simplify * Simplify * Share code * Split into local and remote calls * Add throw * Add throw * Simplify * Simplify * Simplify * Simplify * Simplify * Simplify * Simplify * Simplify * Simplify * Simplify * Simplify * Simplify * Avoid potential disposal exception during MEF shutdown (#77990) Previously, if VS was shutdown before Workspace.EnsureEventListeners was invoked, we would cause an exception during MEF disposal. This would happen because our workspace disposal would attempt to get a service that hadn't already been cached, and thus would ask MEF to compose the item during MEF disposal (which doesn't work). Instead, just cache the IWorkspaceEventListenerService for use during the dispose. * Remove unused elements form PublishData.json (#77994) Removing some elements that seem to be left over from how the publish tooling used to work * Reduce main thread switches in VisualStudioProjectFactory.CreateAndAddToWorkspaceAsync (#77920) * Reduce main thread switches in VisualStudioProjectFactory.CreateAndAddToWorkspaceAsync This method is called once for each project in the solution during solution open, and each call was essentially switching to the main thread twice (once explicitly, once via call to _visualStudioWorkspaceImpl.RefreshProjectExistsUIContextForLanguageAsync). For my testing of opening Roslyn.sln, this removed over 600 main thread switches. Instead: 1) Move the first main thread switch to just occur once in an initialization task. This task is joined when CreateAndAddToWorkspaceAsync is invoked to ensure that that initialization is completed. 2) Move the second main thread switch to instead move inside the callback from an ABWQ. The code being called needs the UI thread, but doesn't need to occur synchronously during the call to CreateAndAddToWorkspaceAsync. * Explicitly releasE * Cancellation is fine * Reorder * remove net * docs * docs * remove exception handling * Add public api * lint * Reduce main thread switches in VisualStudioProjectFactory.CreateAndAddToWorkspaceAsync (#77920) (#77998) * Reduce main thread switches in VisualStudioProjectFactory.CreateAndAddToWorkspaceAsync This method is called once for each project in the solution during solution open, and each call was essentially switching to the main thread twice (once explicitly, once via call to _visualStudioWorkspaceImpl.RefreshProjectExistsUIContextForLanguageAsync). For my testing of opening Roslyn.sln, this removed over 600 main thread switches. Instead: 1) Move the first main thread switch to just occur once in an initialization task. This task is joined when CreateAndAddToWorkspaceAsync is invoked to ensure that that initialization is completed. 2) Move the second main thread switch to instead move inside the callback from an ABWQ. The code being called needs the UI thread, but doesn't need to occur synchronously during the call to CreateAndAddToWorkspaceAsync. * make nested * Simplify * Simplify * Simplify * Simplify * Simplify * restore * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250402.2 (#77982) Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.618101 -> To Version 10.0.620202 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Allow null * Simplify * No VoidResult * Make everything non-capturing * 'nameof(T<>)' not supported yet. * Update references * Revert "Another attempt to move MiscellaneousFilesWorkspace to a bg thread (#77983) This reverts commit 2e8fcbe. * fix * Exception work * Update Roslyn.sln * More exception work * Add remote end * Complete the exception handling work * Add docs * docs * Add docs * Rename type * Rename type * update publishdata * Change SolutionState.ProjectStates from an ImmutableDictionary<ProjectId, ProjectState> to an ImmutableArray<ProjectState> (#77971) * *** WIP. Definitely needs supporting speedometer numbers before being considered *** Change SolutionState to keep/expose and ImmutableArray<ProjectState> instead of an IReadOnlyList<ProjectId> By exposing the project states, there is a bunch of code that can be changed to enumerate over that collection rather than over the values of an ImmutableDictionary<ProjectId, ProjectState>. That enumeration is actually *very* expensive. For example, during roslyn load, local etl traces indicates the following CPU usage enumerating this dictionary: Under ComputeDocumentIdsWithFilePath: 2.6 seconds Under GetFirstRelatedDocumentId: 1.1 seconds There is a bit of a tradeoff associated with this change. The IReadOnlyList<ProjectId> could be reused very often, whereas there is much less opportunity for sharing ImmutableArray<ProjectState> when forking the solution state. I expect there to be more memory allocated in the speedometer runs, so I'd like to get a feel on how much worse that is before elevating this PR out of draft status. Additionally, SolutionState had a CWT over a sorted version of this IReadOnlyList<ProjectId>. As mentioned earlier, the sharing of this data is quite a bit less common now that it's an ImmutableArray<ProjectState>. I locally measured the stopwatch time spent always sorting this data in ComputeChecksumsAsync and it was a total of 4 ms (averaged over two separate VS runs) when loading the roslyn sln. The ImmutableDictionary is still present in SolutionState, but it was renamed to indicate that it's primarily useful as a lookup tool, and all references treating it as an enumeration mechanism were changed to instead use the ImmutableArray<ProjectState>. * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250404.2 (#78008) Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.620202 -> To Version 10.0.620402 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Fix VSTypeScript lsp tests * localize servicehub service name * Simplify workspace initialization in the LSP server Right now we have the logic for which analyzers get added to a workspace in Program.cs, which calls a special method that specifically initializes the main host workspace. This refactors it so that could be used to initialize any workspace, and removes any tricky ordering problems that might happen by using cleaner MEF imports when we have them. * Update test * Update dependencies from https://github.com/dotnet/arcade build 20250404.5 Microsoft.SourceBuild.Intermediate.arcade , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.XliffTasks From Version 9.0.0-beta.25164.2 -> To Version 9.0.0-beta.25204.5 * Update maintenance-packages versions (#78005) * Update maintenance-packages versions * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250404.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.620202 -> To Version 10.0.620402 * Address feedback --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Fix EA layering for Razor.ExternalAccess (#77927) Moves things to /Shared to ship in both MS.CA.EA.Razor and MS.CA.EA.Razor.Features At some point we can rename MS.CA.EA.Razor to MS.CA.EA.Razor.EditorFeatures but I didn't bother with that in this PR Shipping in both to not require dual insertion. We can fix this after a consuming change in Razor is inserted * Delete * Update PublishData.json after VS snap to rel/d17.14 * Handle nameof(indexer) in ref analysis (#78015) * Make a couple of features non-experimental * Ensure external access extensions package gets codebase * Feedback * Add extension message handler service teest * Add extension message handler service test * FIx * Add testS * In progress * Test work * Tests * Add test * More tests * Add tests * Add tests * Add tests * Add test * Add test * Add test * Add test * Add test * Update comment * Disable TransitiveVersioningPinning for RoslynAnalyzers. * Remove dependency on EditorFeatures from lsp tests * Remove dependency on EditorFeatures from codelens layer * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250407.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 10.0.620402 -> To Version 10.0.620702 * Remove dependency on EditorFeatures from build tools * Update dependency versions * Extensions: ref safety analysis (#77967) * Update PublishData.json after VS span to main * In progress * Fix crash in backing field nullability cycle scenario (#77993) * More work * Pass along when an extension was unloaded * Add teset * Add tests * Docs * Simplify * Using alias * Fix * Add tests * Add docs * Update gladstone/roslyn api * Simplify channels in SemanticSearch * Semantic Search: Add support for async queries and FAR tool (#77906) * Simplify further * Simplify further * Localize exception messages * Docs * Docs * Docs * named args * named args * Docs * Move type * Docs * Fix name * Compile just for NET * Unseal LSP types (#78041) * Simplify analyzer api * Update RoslynAnalyzer package projects with dependencies This will fix issues with transitive pinning during source builds. * Ensure LSP uses actual signature help trigger characters * Improve detection of code whose updates may not have effect (#78009) * Do not return metadata names for document symbols * Fix nullability warnings * Review feedback * Remove EditorFeatures from OOP (#78069) * Add back EA.Razor for servicing branches publishdata is pulled from main always. this needs to be present even though the package no longer exists for servicing branches * Remove unused ISemanticSearchWorkspaceHost (#78083) * Split query execution into compile and execute calls (#78081) * Update resourceManagement.yml (#77948) * Expression trees: support optional arguments and named arguments in parameter order (#77972) * Update ignored directives public API (#77968) * Remove experiment * Add Content token to IgnoredDirectiveTriviaSyntax * Add Content token to ShebangDirectiveTriviaSyntax * Fixup public API * Fixup semantic search API list * Fix CLI flag in error message * Remove Update API * Update SemanticSearch API list --------- Co-authored-by: Matteo Prosperi <[email protected]> Co-authored-by: Jason Malinowski <[email protected]> Co-authored-by: Cyrus Najmabadi <[email protected]> Co-authored-by: Cyrus Najmabadi <[email protected]> Co-authored-by: Julien Couvreur <[email protected]> Co-authored-by: Jan Jones <[email protected]> Co-authored-by: Matteo Prosperi <[email protected]> Co-authored-by: Todd Grunke <[email protected]> Co-authored-by: Jared Parsons <[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: David Barbet <[email protected]> Co-authored-by: Carlos Sánchez López <[email protected]> Co-authored-by: Andrew Hall <[email protected]> Co-authored-by: Charles Stoner <[email protected]> Co-authored-by: Tomas Matousek <[email protected]> Co-authored-by: Joey Robichaud <[email protected]> Co-authored-by: Rikki Gibson <[email protected]> Co-authored-by: Tomáš Matoušek <[email protected]>
This was referenced Apr 19, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 #77909
Punted some missing/unexpected nullable warning issues to #77991