Improve detection of invalid references for implicitly typed expression variables declared within implicit object creation expressions. #80546

AlekseyTs · 2025-10-02T16:06:10Z

…ject creation should be expanded to an argument list immediately enclosing the object creation Fixes dotnet#80518.

CyrusNajmabadi · 2025-10-02T16:23:26Z

Fix seems reasonable to me. I'm curious if this needs LDM check? Or does this fall out from the rules of the lang already spec'ed?

--

Note: the above is not blocking. I think addressing the crash is important. Just not sure if this means we compile a subset of what the lang thinks should be legal.

CyrusNajmabadi · 2025-10-02T16:27:22Z

src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs

+            while (forbiddenZoneOpt?.Parent is ImplicitObjectCreationExpressionSyntax { Parent: ArgumentSyntax { Parent: BaseArgumentListSyntax expanded } })
+            {
+                forbiddenZoneOpt = expanded;
+            }


so, this seems reasonable to me. i'm curious what the right path is when we do with(...) elements. IN other words, we likely will want to expand this to cover [new(out var x), x]. This code seems fairly straightforward to update. But it seems a bit fragile in that i don't get a good sense of what the general rule should be for something being updated here. #Closed

Thanks. I'll see if these scenarios need special treatment too.

CyrusNajmabadi · 2025-10-02T16:28:48Z

src/Compilers/CSharp/Test/Semantic/Semantics/ImplicitObjectCreationTests.cs

+                // (9,9): error CS0103: The name 'M1' does not exist in the current context
+                //         M1(new(out var x), x);
+                Diagnostic(ErrorCode.ERR_NameNotInContext, "M1").WithArguments("M1").WithLocation(9, 9),
+                // (9,28): error CS8196: Reference to an implicitly-typed out variable 'x' is not permitted in the same argument list.


diagnostic is a bit incorrect now. but i suppose it's still clear enough?

it seems like it may need tweaking with collection expression work. perhaps make it vaguer as "reference to implicitly-typed out variable 'x' is not permitted here"? #Closed

diagnostic is a bit incorrect now. but i suppose it's still clear enough?

Looks sufficiently good to me for the given scenarios.

AlekseyTs · 2025-10-02T16:58:54Z

does this fall out from the rules of the lang already spec'ed?

I think it does. An implicit object creation expression is converted to its target type after overload resolution succeeds, types of any out vars declared within the object creation argument list cannot be inferred until then. Therefore, the language cannot know the type of the vars referenced in the same argument list (the type is needed to overload resolution before the new() is converted, but the new() needs overload resolution to complete in order to get converted).

CyrusNajmabadi · 2025-10-02T17:05:38Z

I think it does. An implicit object creation expression is converted to its target type after overload resolution succeeds, types of any out vars declared within the object creation argument list cannot be inferred until then. Therefore, the language cannot know the type of the vars referenced in the same argument list (the type is needed to overload resolution before the new() is converted, but the new() needs overload resolution to complete in order to get converted).

That makes perfect sense. Thanks.

…on variables declared within implicit object creation expressions.

Obsolete

CyrusNajmabadi · 2025-10-08T21:19:16Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+                //
+                // For simplicity, we may assign nodes that themselves don't represent expressions to 'typeIsKnownAfterBinding'. 
+
+                SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealind with an argument list,


Suggested change

SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealind with an argument list,

SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealing with an argument list,

``` #Closed

CyrusNajmabadi · 2025-10-08T21:19:42Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+                                                                                // the type is known after an overload resolution for an operation 
+                                                                                // owning the argument list is performed and the arguments are converted
+                                                                                // accordingly. But for our purposes simply pretending that this happens
+                                                                                // when we bind the argument list itself works pretty well.


the formatting of this comment leaves a lot to be desired. Not a fan of multiline done this way after the actual statement. #Closed

CyrusNajmabadi · 2025-10-08T21:23:13Z

Can review fully tomorrow. Thanks.

AlekseyTs · 2025-10-08T22:33:12Z

@dotnet/roslyn-compiler Please review

AlekseyTs · 2025-10-10T15:31:37Z

@dotnet/roslyn-compiler Please review

AlekseyTs · 2025-10-13T15:28:55Z

@jcouv, @dotnet/roslyn-compiler Please review

jcouv · 2025-10-13T22:50:18Z

src/Compilers/CSharp/Portable/Symbols/LocalSymbol.cs

-        internal virtual ErrorCode ForbiddenDiagnostic => ErrorCode.ERR_VariableUsedBeforeDeclaration;
+        /// </param>
+        /// <returns></returns>
+        internal virtual bool IsForbiddenReference(SyntaxNode reference, out ErrorCode forbiddenDiagnostic)


nit: we only care about this for SourceLocalSymbol and all the call-sites already do such a type check. Consider moving this down into SourceLocalSymbol
Then the call-sites can do if (localSymbol is SourceLocalSymbol { IsVar : true } sourceLocal && sourceLocal.IsForbiddenReference(...)) #Closed

jcouv · 2025-10-13T23:02:45Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+                    // inside an argument of a 'new()' have known type before the 'new()' is converted. For example,
+                    // the following code is legal because of that:
+                    // 
+                    //      M1(new() (M2(out var x)), x);


()

Are these extra parens intentional? #Closed

Are these extra parens intentional?

No. Will remove

jcouv · 2025-10-14T15:55:33Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+                //
+                // For simplicity, we may assign nodes that themselves don't represent expressions to 'typeIsKnownAfterBinding'. 
+
+                SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealind with an argument list,


dealind

typo: dealing #Closed

AlekseyTs · 2025-10-17T14:03:22Z

@dotnet/roslyn-compiler Please review

AlekseyTs · 2025-10-20T15:52:20Z

@jcouv, @dotnet/roslyn-compiler Please review

jcouv · 2025-10-20T16:18:39Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

+        {
+            get
+            {
+                return field ??= new ExtendedErrorTypeSymbol(this, name: "var", arity: 0, errorInfo: null, variableUsedBeforeDeclaration: true);


Is there any risk of race condition (two threads concurrently see a null field and decide to set, but to different values, which would mess up downstream phases which check for singleton value)? #Closed

Is there any risk of race condition (two threads concurrently see a null field and decide to set, but to different values, which would mess up downstream phases which check for singleton value)?

Good observation. I'll make an adjustment.

jcouv · 2025-10-20T17:08:28Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+        private TypeWithAnnotations.Boxed? _type;
+
+        // Please don't use thread local storage widely. This should be one of only a few uses.
+        [ThreadStatic] private static PooledHashSet<LocalTypeInferenceInProgressKey>? s_LocalTypeInferenceInProgress;


I didn't understand why we need two data structures and why we need to track syntax nodes (LocalTypeInferenceInProgressKey.Reference and _forbiddenReferences).
Why isn't tracking the set of "locals being bound" (a PooledHashSet<SourceLocalSymbol> or ConsList<SourceLocalSymbol>) and saving the error type into _type sufficient to break the cycles? #Resolved

Without going to much into details:

Only specific references are illegal, other references are fine

If we ignore illegal references (that is what we used to do, and this is what I think we should keep doing), the inference might still succeed (quite possibly as part of an error recovery), therefore failing the inference once we detect an invalid reference is undesirable.

FWIW, I gave the alternative design a quick try to understand the problem better, and now understand your second bullet: In a scenario like var x1 = Dummy(TakeOutParam(true, out var x1), x1); (Scope_LocalDeclarationStmt_03) we'd set the type for x1 to object (because of coercion to parameter type from TakeOutParam) before we detect the circularity and thus before we get to record an error type.

jcouv

Done with review pass (commit 11). May take another look after clarifying comment is addressed. Thanks

jcouv

LGTM Thanks (commit 11)

AlekseyTs · 2025-10-20T19:28:55Z

@dotnet/roslyn-compiler For a second review

AlekseyTs · 2025-10-21T13:34:00Z

@dotnet/roslyn-compiler For a second review

RikkiGibson · 2025-10-21T17:58:04Z

Taking a look today.

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

RikkiGibson · 2025-10-21T20:13:11Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+                    if (_forbiddenReferences?.Contains(reference) == true)
+                    {
+                        diagnostics.Add(ForbiddenDiagnostic, reference.Location, reference);
+                        return TypeWithAnnotations.Create(this.DeclaringCompilation.ImplicitlyTypedVariableUsedInForbiddenZoneType);


Should we SetTypeWithAnnotations at this point? Otherwise, is there a risk that the next caller to ask about the type, will cause the work of detecting the forbidden reference to be done over again? #Resolved

It looks like SetTypeWithAnnotations would not set the ImplicitlyTapedVariableUsedInForbiddenZoneType anyway. So, this suggestion might be irrelevant.

Should we SetTypeWithAnnotations at this point?

I intentionally don't do that. Very often, even in presence of an illegal reference, during error recovery we are able to infer the type of the local pretty accurately. Basically I am trying maintain the previous behavior where the fact of detecting an illegal reference has no impact on the inferred type.

Otherwise, is there a risk that the next caller to ask about the type, will cause the work of detecting the forbidden reference to be done over again?

We should not be doing the work again for the same location since we recorded it in _forbiddenReferences, which is checked on entry and we return right away. It is possible that we will do the work for another location until we infer the type or explicitly fail the inference. Since the error should not be reported for legal locations, even in presence of an illegal reference, we have to check each distinct location. Eventually all of them get recorded (it is not an unbound set) and we complete the inference. After that we will not attempt the inference for the same local.

I believe I am following now, thanks.

Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>

* upstream/main: (123 commits) Fix SafeContext of Span-valued collection expressions to match specification (dotnet#80684) Improve detection of invalid references for implicitly typed expression variables declared within implicit object creation expressions. (dotnet#80546) Add test demonstrating behavior of ToMinimalDisplayString. (dotnet#80757) Only set DOTNET_HOST_PATH if something was installed (dotnet#80842) Clean up a Razor external access service (dotnet#80830) Remove unused statement (dotnet#80823) Allow foreach on typed null enumerables (dotnet#80783) Update documentation for DeclaringSyntaxReferences and Locations to clarify partial members behavior (dotnet#80704) Fix issue converting an auto prop to a full prop when 'field' and 'initializers' are involved Rename childIsSimple to innerExpressionHasPrimaryPrecedence and add more test cases Remove placeholder WorkItem attributes from new tests Fix RemoveUnnecessaryParentheses to detect simple expressions in bitwise operations [main] Update dependencies from dotnet/arcade (dotnet#80828) Fix ITypeSymbol.BaseType documentation for type parameters (dotnet#80770) soft-select select camelcase matched item if user might be typing an undefined type parameter (dotnet#80809) Allow semantic tokens in Razor to be better behaved (dotnet#80815) Rebase Remove using Update test Add fix all test ...

Forbidden zone for an out var used as an argument of an implicit ob…

bfb2678

…ject creation should be expanded to an argument list immediately enclosing the object creation Fixes dotnet#80518.

AlekseyTs requested a review from a team as a code owner October 2, 2025 16:06

AlekseyTs added Area-Compilers Feature - Target-Typed New `new (args)` gets the created type inferred from context labels Oct 2, 2025

CyrusNajmabadi reviewed Oct 2, 2025

View reviewed changes

CyrusNajmabadi previously approved these changes Oct 2, 2025

View reviewed changes

AlekseyTs marked this pull request as draft October 2, 2025 21:29

AlekseyTs closed this Oct 3, 2025

Improve detection of invalid references for implicitly typed expressi…

23fe570

…on variables declared within implicit object creation expressions.

AlekseyTs reopened this Oct 8, 2025

Merge remote-tracking branch 'upstream/main' into Issue80518

4cba33a

AlekseyTs marked this pull request as ready for review October 8, 2025 21:12

AlekseyTs requested a review from CyrusNajmabadi October 8, 2025 21:12

CyrusNajmabadi reviewed Oct 8, 2025

View reviewed changes

AlekseyTs requested a review from jcouv October 9, 2025 13:15

jcouv self-assigned this Oct 13, 2025

jcouv reviewed Oct 13, 2025

View reviewed changes

jcouv reviewed Oct 14, 2025

View reviewed changes

AlekseyTs added 2 commits October 15, 2025 22:09

Fix release build

d62d00e

Formatting

bfab9e5

AlekseyTs requested review from a team and jcouv October 16, 2025 15:19

Improve thread safety

6a65cfe

jcouv reviewed Oct 20, 2025

View reviewed changes

Improve thread safety

66973ce

jcouv reviewed Oct 20, 2025

View reviewed changes

jcouv approved these changes Oct 20, 2025

View reviewed changes

AlekseyTs requested review from a team and jcouv October 20, 2025 19:24

RikkiGibson reviewed Oct 21, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Oct 21, 2025

View reviewed changes

RikkiGibson approved these changes Oct 21, 2025

View reviewed changes

Update src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

c5ad010

Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>

AlekseyTs enabled auto-merge (squash) October 22, 2025 13:38

AlekseyTs merged commit f230001 into dotnet:main Oct 22, 2025
23 of 24 checks passed

dotnet-policy-service bot added this to the Next milestone Oct 22, 2025

dotnet-maestro bot mentioned this pull request Oct 23, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3061

Closed

This was referenced Oct 25, 2025

[Automated] PRs inserted in VS build feature.polytype-11124.148 #80896

Closed

[Automated] PRs inserted in VS build main-11124.32 #80897

Closed

[Automated] PRs inserted in VS build feature.debugger.main-11127.165 #80926

Closed

dotnet-maestro bot mentioned this pull request Oct 29, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3147

Open

	SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealind with an argument list,
	SyntaxNode? typeIsKnownAfterBinding = nodeEnclosingDeclaration; // Strictly speaking, when we are dealing with an argument list,
	``` #Closed

Improve detection of invalid references for implicitly typed expression variables declared within implicit object creation expressions. #80546

Improve detection of invalid references for implicitly typed expression variables declared within implicit object creation expressions. #80546

Uh oh!

Conversation

AlekseyTs commented Oct 2, 2025

Uh oh!

CyrusNajmabadi commented Oct 2, 2025

Uh oh!

CyrusNajmabadi Oct 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 2, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Oct 2, 2025

Uh oh!

CyrusNajmabadi commented Oct 2, 2025

Uh oh!

CyrusNajmabadi Oct 8, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 8, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Oct 8, 2025

Uh oh!

AlekseyTs commented Oct 8, 2025

Uh oh!

AlekseyTs commented Oct 10, 2025

Uh oh!

AlekseyTs commented Oct 13, 2025

Uh oh!

jcouv Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Oct 17, 2025

Uh oh!

AlekseyTs commented Oct 20, 2025

Uh oh!

jcouv Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 20, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

CyrusNajmabadi Oct 2, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Oct 2, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Oct 8, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Oct 8, 2025 •

edited by jcouv

Loading

jcouv Oct 13, 2025 •

edited

Loading

jcouv Oct 13, 2025 •

edited

Loading

jcouv Oct 14, 2025 •

edited

Loading

jcouv Oct 20, 2025 •

edited

Loading

jcouv Oct 20, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Oct 20, 2025 •

edited

Loading

RikkiGibson Oct 21, 2025 •

edited by AlekseyTs

Loading

RikkiGibson Oct 21, 2025 •

edited

Loading