Break cycle for lambda using field keyword with inferred return type #79995
Conversation
RikkiGibson
changed the title
| } | ||
| } | ||
|
|
||
| internal readonly struct GetterNullResilienceData(SynthesizedBackingFieldSymbol field, NullableAnnotation assumedAnnotation) |
There was a problem hiding this comment.
This is pretty much the same as the tuple we had before but it felt like as the usage was starting to leak into more places, it was better off as a named type.
There was a problem hiding this comment.
consider documenting this type. what is represents (including examples) when when it would be used.
There was a problem hiding this comment.
as examples, i don't know what 'assumed' means in this context at all.
There was a problem hiding this comment.
Updating docs and updated spec PR dotnet/csharplang#9184
One thing tha twould be helpful in the OP is to state if this follows the spec in terms of correctness, or if we're adding some sort of heuristic to get out of a bad situation, but it means we don't align with the lang. It's unclear to me from your explanation which path we're on (or something else entirely). Thanks! |
| /// only needed when nullability is inferred. | ||
| /// </summary> | ||
| public TypeWithAnnotations GetInferredReturnType(ConversionsBase? conversions, NullableWalker.VariableState? nullableState, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, out bool inferredFromFunctionType) | ||
| public TypeWithAnnotations GetInferredReturnType(ConversionsBase? conversions, NullableWalker.VariableState? nullableState, NullableWalker.GetterNullResilienceData? getterNullResilienceData, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, out bool inferredFromFunctionType) |
There was a problem hiding this comment.
consider updating the docs here. To me, the name GetterNullResilienceData getterNullResilienceData isn't immediately apparent as to what purpose it serves or when it would be used.
|
|
||
| private T MethodReturningLambda<T>(Func<T> thisGet) => thisGet(); | ||
| } | ||
| """; |
There was a problem hiding this comment.
thank you for indenting.
| SyntaxNode syntax, | ||
| DiagnosticBag diagnostics, | ||
| (SourcePropertyAccessorSymbol getter, SynthesizedBackingFieldSymbol field, NullableAnnotation assumedNullableAnnotation)? getterNullResilienceData = null) | ||
| (SourcePropertyAccessorSymbol symbol, GetterNullResilienceData getterNullResilienceData)? symbolAndGetterNullResilienceData = null) |
There was a problem hiding this comment.
unrelated to this particular change, but it does seem sa bit odd to me to have all this data just stashed as an optional variable at the end of this (seemingly) major entrypoint into nullable walking.
There was a problem hiding this comment.
The usages of this overload are somewhat specialized and include:
- attributes
- parameter default values
- computing the backing field inferred nullability
Only in the latter case is a value actually passed, so that we can indicate that we are trying to do the "not-annotated pass" versus the "annotated" pass, so that caller can decide if the associated property is null-resilient.
There was a problem hiding this comment.
Theoretically the backing field case could have its own overload or call one of the more general overloads directly, it just didn't strike me as a valuable change, it works well enough to have a parameter that only one caller passes a value for.
Note: this feedback applies to prs in general. An occasional comment explaining the supporting pieces, and then how it comes together can be very helpful. Thanks! |
|
Have updated PR description with more clear information about what the problem is and why this PR fixes it. |
|
@dotnet/roslyn-compiler for review |
1 similar comment
|
@dotnet/roslyn-compiler for review |
|
|
||
| public class DemoCscBreaks | ||
| { | ||
| public List<string> WillBreak => MethodReturningLambda(() => field ?? new List<string>()); |
There was a problem hiding this comment.
Consider verifying the final annotation for field.
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/78932")] | ||
| public void Lambda_03() |
There was a problem hiding this comment.
Let's add some more variations, such as setters, and instances where the lambda will incur nullable warnings.
* upstream/main: (201 commits) Handle extension blocks in CLS compliance checker (#80251) Fix Simplify Simplify Simplify Simplify Simplify Add doc for GetInferredNullableAnnotation (#80245) Rename files to match type within Implement ref local hoisting in runtime async. Closes #79763. Pull ref initialization hoisting into a reusable class for use in runtime async REvert Break cycle for lambda using field keyword with inferred return type (#79995) Simplify tests Simplify Fixup New extension Extensions: add SyntaxGenerator support and AssociatedExtensionImplementation API (#80170) In process Support 'add using' for modern extension methods ...
* upstream/main: (1725 commits) Handle extension blocks in CLS compliance checker (dotnet#80251) Fix Simplify Simplify Simplify Simplify Simplify Add doc for GetInferredNullableAnnotation (dotnet#80245) Rename files to match type within Implement ref local hoisting in runtime async. Closes dotnet#79763. Pull ref initialization hoisting into a reusable class for use in runtime async REvert Break cycle for lambda using field keyword with inferred return type (dotnet#79995) Simplify tests Simplify Fixup New extension Extensions: add SyntaxGenerator support and AssociatedExtensionImplementation API (dotnet#80170) In process Support 'add using' for modern extension methods ...
* upstream/main: (131 commits) Handle extension blocks in CLS compliance checker (dotnet#80251) Fix Simplify Simplify Simplify Simplify Simplify Add doc for GetInferredNullableAnnotation (dotnet#80245) Rename files to match type within Implement ref local hoisting in runtime async. Closes dotnet#79763. Pull ref initialization hoisting into a reusable class for use in runtime async REvert Break cycle for lambda using field keyword with inferred return type (dotnet#79995) Simplify tests Simplify Fixup New extension Extensions: add SyntaxGenerator support and AssociatedExtensionImplementation API (dotnet#80170) In process Support 'add using' for modern extension methods ...
5 participants
Closes #78932
#78932 (comment)
What's happening is: in some cases we do a "child" nullable analysis of a lambda to figure out its return type. If we analyze a lambda which uses the field keyword, we will likely ask for its inferred nullable annotation, which launches another analysis of the get accessor, causing us to analyze the lambda return type again. We keep calling thru this cycle until we blow the stack.
What we need to do to fix it is, pass down any context around the getter null resilience analysis that may be currently taking place. In which case we can avoid kicking off a new inference of the backing field's nullable annotation, and avoid getting into a cycle.
One alternative here might be to include
GetterNullResilienceDatainVariableStateitself.VariableStateseems to be used to propagate flow state down to analysis of lambdas and local functions, as well as propagating state from "field/property initializers analysis" to constructor analysis.This state is not really flow state, but it's close.. the nullable annotation for the
fielddetermines thefield's initial state, and we came pretty close to spec'ing this feature in terms of what is the initial flow state of the field rather than what is its "inferred annotation".