Extensions: bind unconditionally of LangVer #78947

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

Merged

jcouv merged 4 commits into dotnet:main from jcouv:extensions-langver2

Jun 17, 2025

Member

jcouv commented Jun 12, 2025 •

edited

Loading

Relates to test plan #76130

github-actions bot added the Area-Compilers label

jcouv self-assigned this

jcouv added the Feature - Extension Everything label

jcouv force-pushed the extensions-langver2 branch from 223609d to dde645c Compare

June 13, 2025 04:27


          Extensions: bind unconditionally of LangVer

4839b84

jcouv force-pushed the extensions-langver2 branch from dde645c to 4839b84 Compare

June 13, 2025 05:45

AlekseyTs reviewed

View reviewed changes

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

    
                          }

                          // This is what BindInvocationExpressionContinued is doing in terms of reporting diagnostics and detecting a failure

                          static bool bindInvocationExpressionContinued(

Contributor

AlekseyTs Jun 13, 2025 •

edited

Loading

bindInvocationExpressionContinued

It is not clear why this local function is removed. As the comment states, it is supposed to do what BindInvocationExpressionContinued would do. It looks like BindInvocationExpressionContinued is still there and didn't even change. #Closed

Member Author

jcouv Jun 13, 2025 •

edited

Loading

The local function was removed because the second half of the code is unreachable. We only have one caller for the local function and we always get here with !result.Succeeded. I'll file an issue to follow-up on this instead, as to not impact extensions progress.
Filled #78954

Contributor

AlekseyTs Jun 13, 2025

It is fine to adjust the body, but it was an intent to have the local function not for the purpose of code reuse, but for the purpose to make it easier to keep two different code paths in sync.


          Address feedback

c53a2f3

jcouv marked this pull request as ready for review

June 13, 2025 18:30

jcouv requested a review from a team as a code owner

June 13, 2025 18:31

333fred approved these changes

View reviewed changes

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs Outdated

    
                                  ReportDiagnosticsIfObsolete(diagnostics, method, node, hasBaseReceiver: false);

                                  ReportDiagnosticsIfUnmanagedCallersOnly(diagnostics, method, node, isDelegateConversion: false);

                                  Debug.Assert(!IsDisallowedExtensionInOlderLangVer(method));

Contributor

AlekseyTs Jun 16, 2025 •

edited

Loading

Debug.Assert(!IsDisallowedExtensionInOlderLangVer(method));

I suggest reverting this change in order to avoid unnecessary merge conflicts. This code path is specific to operators and operators are handled in a feature branch. BTW, operators must always be reported, the assert is misleading. #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs Outdated

    
                                  ReportDiagnosticsIfObsolete(diagnostics, method, node, hasBaseReceiver: false);

                                  ReportDiagnosticsIfUnmanagedCallersOnly(diagnostics, method, node, isDelegateConversion: false);

                                  Debug.Assert(!IsDisallowedExtensionInOlderLangVer(method));

Contributor

AlekseyTs Jun 16, 2025 •

edited

Loading

Debug.Assert(!IsDisallowedExtensionInOlderLangVer(method));

Same comment as above. #Closed

AlekseyTs reviewed

View reviewed changes

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

    
                      internal static bool IsDisallowedExtensionInOlderLangVer(MethodSymbol symbol)

                      {

                          return symbol.GetIsNewExtensionMember() && symbol.IsStatic;

Contributor

AlekseyTs Jun 16, 2025 •

edited

Loading

symbol.GetIsNewExtensionMember() && symbol.IsStatic;

It feels like the condition is somewhat inaccurate. Instance ordinary methods are allowed, other kinds of methods are disallowed. #Closed

Member Author

jcouv Jun 16, 2025

I'm reluctant to add logic that can't be exercised in this branch, but if it'll make the merge easier, seems reasonable

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs Outdated

                       CompileAndVerify(comp, expectedOutput: "(42, 43)").VerifyDiagnostics();
+                      comp = CreateCompilation(src, references: [libRef], parseOptions: TestOptions.Regular13);
+                      comp.VerifyEmitDiagnostics();

Contributor

AlekseyTs Jun 16, 2025 •

edited

Loading

comp.VerifyEmitDiagnostics();

Consider doing the same verification as we do for Preview version above. #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs Outdated

+                      comp.VerifyEmitDiagnostics();
+                      comp = CreateCompilation(src, references: [libRef], parseOptions: TestOptions.RegularNext);
+                      comp.VerifyEmitDiagnostics();

Contributor

AlekseyTs Jun 16, 2025 •

edited

Loading

comp.VerifyEmitDiagnostics();

Same suggestion #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs Outdated

+                          Diagnostic(ErrorCode.ERR_FeatureInPreview, "Property").WithArguments("extensions").WithLocation(2, 12));
+                      comp = CreateCompilation(src, references: [libRef], parseOptions: TestOptions.RegularNext);
+                      comp.VerifyEmitDiagnostics();

Contributor

AlekseyTs Jun 16, 2025 •

edited

Loading

comp.VerifyEmitDiagnostics();

CompileAndVerify(comp, expectedOutput: "property").VerifyDiagnostics();? #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs Outdated

+                          Diagnostic(ErrorCode.ERR_FeatureInPreview, "Property").WithArguments("extensions").WithLocation(1, 15));
+                      comp = CreateCompilation(src, references: [libRef], parseOptions: TestOptions.RegularNext);
+                      comp.VerifyEmitDiagnostics();

Contributor

AlekseyTs Jun 16, 2025 •

edited

Loading

comp.VerifyEmitDiagnostics();

CompileAndVerify(comp, expectedOutput: "property").VerifyDiagnostics(); #Closed

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs Outdated

+                          Diagnostic(ErrorCode.ERR_FeatureInPreview, "Property").WithArguments("extensions").WithLocation(1, 20));
+                      comp = CreateCompilation(src, references: [libRef], parseOptions: TestOptions.RegularNext);
+                      comp.VerifyEmitDiagnostics();

Contributor

AlekseyTs Jun 16, 2025 •

edited

Loading

comp.VerifyEmitDiagnostics();

CompileAndVerify(comp, expectedOutput: "property").VerifyDiagnostics();? #Closed

Contributor

AlekseyTs commented Jun 16, 2025

Done with review pass (commit 2)

Julien Couvreur added 2 commits

June 16, 2025 12:33


          Address feedback

77a0cdc


          Address feedback

75011c8

AlekseyTs approved these changes

View reviewed changes

Contributor

AlekseyTs left a comment

LGTM (commit 4)

jcouv merged commit d9a48df into dotnet:main

24 checks passed

jcouv deleted the extensions-langver2 branch

June 17, 2025 16:53

dotnet-policy-service bot added this to the Next milestone

CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request


          Fix version

9af286b

Fix more

Update versions

Fix more

In progress

Flip

Fix

Fix

remove wrapper

Fix

Fix

in progress

Fix DeclarationKind layering

fix

One building

In progress

Breaking out refactoring helpers into core api vs service

VB side

In progress

Move type

workaround

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CSharpCompilerExtensions.projitems

Fix SubText ctor parameter verification. (dotnet#78989)

* Fix SubText ctor parameter verification.

This ctor currently throws given a zero length span at the end (and only the end) of the subtext. This evidently became far more prevalent with this (fairly) recent change to optimize newline information in our sourcetext classes. (dotnet#74728)

It doesn't make to allow zero length spans at all locations but at the end of the subtext, so the fix is just to allow construction in that case too.

Big props to Manish Vasani for providing a very actionable repro for this.

Fixes dotnet#78985.

Note there is a potentially related issue outlined in dotnet#76225 that looks attributable to the PR above too. It possible that this addresses that issue too, but I'm not completely convinced.

Extensions: bind unconditionally of LangVer (dotnet#78947)

Extensions: use specific tracking issues for different areas (second pass) (dotnet#78971)

Fix nullable analysis involving extensions and collection expressions (dotnet#78926)

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CSharpCompilerExtensions.projitems

in progrss

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/VisualBasicCompilerExtensions.projitems

extensions

Fix

RootNamespace

Fixes

Fixes

IN progress

in progress

CyrusNajmabadi mentioned this pull request

Have RoslynAnalyzers reference shared utilities/extensions from Roslyn layer itself. #79030

Merged

jcouv mentioned this pull request

Extension Member Properties: Properties Not Available in Outside Assembly #78719

Closed

dotnet-bot mentioned this pull request

[Automated] PRs inserted in VS build main-10719.37 #79072

Closed

This was referenced Jun 21, 2025

[Automated] PRs inserted in VS build feature.debugger.main-10720.140 #79090

Closed

[Automated] PRs inserted in VS build feature.vc.slnExplorerSymbolsProviderRewrite-10725.120 #79142

Closed

dotnet-bot mentioned this pull request

[Automated] PRs inserted in VS build feature.Wix5-10728.31 #79190

Closed

ToddGrun added a commit to ToddGrun/roslyn that referenced this pull request


          Revert "Extensions: bind unconditionally of LangVer (dotnet#78947)"

67052db

This reverts commit d9a48df.

RikkiGibson modified the milestones: Next, 18.0 P1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Extension Everything