Check language version for more scenarios #73546

AlekseyTs · 2024-05-17T17:01:05Z

No description provided.

AlekseyTs · 2024-05-17T17:01:53Z

@cston, @jjonescz, @dotnet/roslyn-compiler Please review

cston · 2024-05-17T19:09:05Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

@@ -5853,9 +5853,9 @@ void verify(ModuleSymbol m)
                Assert.Equal("I1", s1.InterfacesNoUseSiteDiagnostics().Single().ToTestDisplayString());
            }

-            CreateCompilation(src, targetFramework: TargetFramework.Net80, parseOptions: TestOptions.RegularNext).VerifyDiagnostics();
+            CreateCompilation(src, targetFramework: TargetFramework.Net70, parseOptions: TestOptions.RegularNext).VerifyDiagnostics();


TargetFramework.Net70

Was there a reason to change this test to .NET7? #Resolved

Was there a reason to change this test to .NET7?

Just to make the scenario "stronger"

cston · 2024-05-17T19:39:49Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+}
+";
+            var comp2 = CreateCompilation(src2, references: [comp1.ToMetadataReference()], targetFramework: s_targetFrameworkSupportingByRefLikeGenerics, parseOptions: TestOptions.Regular12);
+            comp2.VerifyEmitDiagnostics(


VerifyEmitDiagnostics

Why do we report an error here for -langversion:12 using IDisposable, but we don't report an error for the -langversion:12 case using IEnumerable<int> in Foreach_IEnumerableT_LanguageVersion_03? #Resolved

Why do we report an error here for -langversion:12 using IDisposable, but we don't report an error for the -langversion:12 case using IEnumerable<int> in Foreach_IEnumerableT_LanguageVersion_03?

Semantic analysis didn't rely and still doesn't rely on conversion classification for generic IEnumerable case. I will check behavior in main if it runs into problems elsewhere and will follow up separately, if needed.

Besides, Foreach_IEnumerableT_LanguageVersion_03 doesn't dispose a ref struct

cston · 2024-05-17T19:58:31Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+}
+";
+            var comp2 = CreateCompilation(src2, references: [comp1.ToMetadataReference()], targetFramework: s_targetFrameworkSupportingByRefLikeGenerics, parseOptions: TestOptions.Regular12);
+            comp2.VerifyEmitDiagnostics();


comp2.VerifyEmitDiagnostics();

Why does this case succeed but the corresponding -langversion:12 case in Foreach_IEnumerable_LanguageVersion_03 fails? #Resolved

Why does this case succeed but the corresponding -langversion:12 case in Foreach_IEnumerable_LanguageVersion_03 fails?

Semantic analysis didn't rely and still doesn't rely on conversion classification for generic IEnumerable case. I will check behavior in main if it runs into problems elsewhere and will follow up separately, if needed.

cston · 2024-05-17T20:02:32Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+    static void Main()
+    {
+        System.Console.Write((new S() { 200, 40, 6 }).P);
+    }


Was the intent to run the code for the success cases? #Resolved

Was the intent to run the code for the success cases?

No. Execution is covered by other tests. I think ObjectCreation_05 in this particular case

AlekseyTs · 2024-05-17T20:56:04Z

@jjonescz, @dotnet/roslyn-compiler For the second review

333fred

Done review pass. Only a couple of minor questions.

333fred · 2024-05-17T21:53:14Z

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

+                if (needSupportForRefStructInterfaces &&
+                    initializerType.ContainingModule != Compilation.SourceModule)
+                {
+                    CheckFeatureAvailability(node, MessageID.IDS_FeatureRefStructInterfaces, diagnostics);
+                }
+


Nit: this check is used in several places. Could we consider extracting a helper CheckFeatureAvailabilityForUsage or soemthing similar? #WontFix

333fred · 2024-05-17T21:59:17Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+        }
+
+        [Fact]
+        public void Using_LanguageVersion_04()


Nit: straight from 2 to 4? #Resolved

Nit: straight from 2 to 4?

There is no requirement to avoid gaps in numbers. There is a reason, however, the number matches the other test that covers similar scenario.

333fred · 2024-05-17T22:00:31Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+}
+";
+
+            var comp1 = CreateCompilation(src1, targetFramework: s_targetFrameworkSupportingByRefLikeGenerics);


Why aren't we checking langversion on implementation? Or is that forthcoming? #Resolved

Why aren't we checking langversion on implementation? Or is that forthcoming?

Because this isn't a goal for this PR. I assume you are referring to a scenario covered in ImplementAnInterface_01.

Yes, thanks for clarifying.

333fred · 2024-05-17T22:01:26Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+        }
+
+        [Fact]
+        public void Foreach_IEnumerableT_LanguageVersion_03()


Nit: straight from 1 to 3? #Resolved

Nit: straight from 1 to 3?

Same response

333fred

Done review pass. Only a couple of minor questi

Check language version for more scenarios

2f577b7

AlekseyTs added Area-Compilers Feature - RefStructInterfaces labels May 17, 2024

AlekseyTs requested review from jjonescz and cston May 17, 2024 17:01

AlekseyTs requested a review from a team as a code owner May 17, 2024 17:01

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label May 17, 2024

cston reviewed May 17, 2024

View reviewed changes

Disable some tests

94537aa

cston approved these changes May 17, 2024

View reviewed changes

AlekseyTs requested a review from a team May 17, 2024 20:55

333fred reviewed May 17, 2024

View reviewed changes

333fred approved these changes May 17, 2024

View reviewed changes

AlekseyTs merged commit 71887d8 into dotnet:features/RefStructInterfaces May 17, 2024
24 checks passed

This was referenced May 21, 2024

[Automated] PRs inserted in VS build main-34920.65 #73605

Closed

[Automated] PRs inserted in VS build feature.debugger.main-34920.221 #73610

Closed

dotnet-bot mentioned this pull request Jun 27, 2024

[Automated] PRs inserted in VS build feature.dotnetVS-35026.97 #74176

Closed

dibarbet mentioned this pull request Jul 25, 2024

Find-All-References results incomplete and unstable dotnet/vscode-csharp#7391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check language version for more scenarios #73546

Check language version for more scenarios #73546

AlekseyTs commented May 17, 2024

AlekseyTs commented May 17, 2024

cston May 17, 2024 •

edited by AlekseyTs

Loading

AlekseyTs May 17, 2024

cston May 17, 2024 •

edited by AlekseyTs

Loading

AlekseyTs May 17, 2024

AlekseyTs May 17, 2024

cston May 17, 2024 •

edited by AlekseyTs

Loading

AlekseyTs May 17, 2024

cston May 17, 2024 •

edited by AlekseyTs

Loading

AlekseyTs May 17, 2024

AlekseyTs commented May 17, 2024

333fred left a comment

333fred May 17, 2024 •

edited by AlekseyTs

Loading

333fred May 17, 2024 •

edited by AlekseyTs

Loading

AlekseyTs May 17, 2024

333fred May 17, 2024 •

edited by AlekseyTs

Loading

AlekseyTs May 17, 2024

333fred May 17, 2024

333fred May 17, 2024 •

edited by AlekseyTs

Loading

AlekseyTs May 17, 2024

333fred left a comment

Check language version for more scenarios #73546

Check language version for more scenarios #73546

Conversation

AlekseyTs commented May 17, 2024

AlekseyTs commented May 17, 2024

cston May 17, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 17, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 17, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 17, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 17, 2024

333fred left a comment

Choose a reason for hiding this comment

333fred May 17, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred May 17, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred May 17, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred May 17, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

cston May 17, 2024 •

edited by AlekseyTs

Loading

cston May 17, 2024 •

edited by AlekseyTs

Loading

cston May 17, 2024 •

edited by AlekseyTs

Loading

cston May 17, 2024 •

edited by AlekseyTs

Loading

333fred May 17, 2024 •

edited by AlekseyTs

Loading

333fred May 17, 2024 •

edited by AlekseyTs

Loading

333fred May 17, 2024 •

edited by AlekseyTs

Loading

333fred May 17, 2024 •

edited by AlekseyTs

Loading