Ref fields feature requires ByRefFields runtime flag by jcouv · Pull Request #62941 · dotnet/roslyn

jcouv · 2022-07-26T07:25:42Z

Relates to test plan #59194

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

333fred · 2022-08-02T16:29:15Z

src/Compilers/VisualBasic/Test/Symbol/SymbolDisplay/SymbolDisplayTests.vb

 }"
            Dim comp = CreateCSharpCompilation(GetUniqueName(), source, parseOptions:=New CSharp.CSharpParseOptions(CSharp.LanguageVersion.Preview))
-            comp.VerifyDiagnostics()
+            comp.VerifyDiagnostics(


Please use AssertTheseDiagnostics #Resolved

Had tried that, but it fails with System.InvalidCastException : Unable to cast object of type 'Microsoft.CodeAnalysis.CSharp.CSharpCompilation' to type 'Microsoft.CodeAnalysis.VisualBasic.VisualBasicCompilation'.

Also, I'd considered getting ride of the diagnostic, by adding a runtime flag, but the C# test hook can't easily be reached from VB tests.
Another option is to skip the test until we have TargetFramework.Net70

Then please at least include what these errors are.

333fred

Done review pass (commit 1)

cston · 2022-08-02T21:37:02Z

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


        private static string IncludeExpectedOutput(string expectedOutput) => IsNet70OrGreater() ? expectedOutput : null;

+        private static MetadataReference MscorlibRefWithoutSharingCachedSymbols


private static MetadataReference MscorlibRefWithoutSharingCachedSymbols

Can this be a static readonly field with initializer so we don't need to copy the value to a local in methods that create more than one compilation? #Resolved

There's only two tests that use this. What are you trying to achieve?

I was mistaken. I thought this was used in more tests, and that all tests could share a common reference.

cston · 2022-08-02T21:46:51Z

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

 }";
-            var comp = CreateCompilation(sourceA, parseOptions: TestOptions.Regular10);
+            var mscorlibWithRefFields = MscorlibRefWithoutSharingCachedSymbols;
+            var comp = CreateByRefFieldsCompilation(sourceA, references: new[] { mscorlibWithRefFields }, parseOptions: TestOptions.Regular10);


CreateByRefFieldsCompilation(sourceA, references: new[] { mscorlibWithRefFields }, parseOptions: TestOptions.Regular10)

Could we hide most of the work in CreateCompilation()?

For instance, add a parameter to CreateCompilation(..., RuntimeFeatures runtimeFeatures, ...) that creates a cloned version of the appropriate corlib with the necessary features enabled, and that cloned version is created at most once and shared with all callers that need the same set of features. #Resolved

cston · 2022-08-03T04:45:32Z

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

+            // RuntimeSupportsByRefFields property for it.
+            var mscorlibRefWithRefFields =
+                ((AssemblyMetadata)((MetadataImageReference)MscorlibRef).GetMetadata()).CopyWithoutSharingCachedSymbols().
+                     GetReference(display: "mscorlib.v4_0_30319.dll");


Can we calculate this once and cache the value, perhaps in CSharpTestBase? #Resolved

Perhaps each caller needs a separate instance in case not all callers set the same runtime features. In that case, consider extracting a helper method in the base class.

jcouv · 2022-08-03T18:13:49Z

@333fred for second review. Thanks

jcouv · 2022-08-04T16:33:31Z

@333fred for second review. Thanks

333fred

Done review pass (commit 9). @jcouv and I need to sync on the approach taken in the tests, as no previous bypass flag has needed this level of modification before.

333fred · 2022-08-04T17:23:28Z

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

-            => RuntimeSupportsFeature(SpecialMember.System_Runtime_CompilerServices_RuntimeFeature__ByRefFields);
+        internal virtual bool RuntimeSupportsByRefFields
+        {
+            get


Nit: please use consistent style between these two one-line methods :). #Resolved

333fred · 2022-08-04T17:30:47Z

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

-            var comp = CreateCompilation(sourceA, parseOptions: TestOptions.Regular10);
+            var mscorlibRefWithRefFields = GetMscorlibRefWithoutSharingCachedSymbols();
+
+            // Note: we use skipUsesIsNullable and skipExtraValidation so that nobody pulls


I'm confused why this is necessary. I wouldn't expect anything to be pulling on the compilation or it's references until the compilation is actually used? Can you clarify why we need to do this for this particular flag, but haven't needed to for any of the other RuntimeSupports bypass flags? #Resolved

We don't need this for this particular flag, but we need it for certain flags such as numeric IntPtr (pattern that I'm following).
I'll remove but we should have a discussion at some point on a universal test hook, so that we don't have to special-case every new runtime feature flag.

I opened an issue (#63223) to sit down and design a general/re-usable CreateCompilation API supporting what we need, including a note on skipUsesIsNullable and skipExtraValidation since some features need those but some don't.

333fred · 2022-08-04T17:35:57Z

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


        private static string IncludeExpectedOutput(string expectedOutput) => IsNet70OrGreater() ? expectedOutput : null;

+        private static IEnumerable<MetadataReference> CopyWithoutSharingCachedSymbols(IEnumerable<MetadataReference> references)


Why is this necessary? It wasn't necessary for any previous feature that set override flags. #Resolved

This is used in two tests that need more than mscorlib. For example, one needs dynamic machinery.
Unless those previous features tests such combinations, they would not run into this situation.
I can't say that I understand CopyWithoutSharingCachedSymbols well enough to say there isn't a better way to do this. I'm happy to open a tracking issue to re-discuss when Aleksey is back.

Filed #63222 to review usages of CopyWithoutSharingCachedSymbols()

jcouv self-assigned this Jul 26, 2022

ghost added the Area-Compilers label Jul 26, 2022

jcouv force-pushed the runtime-flag branch 3 times, most recently from f0dedc9 to fb5fcda Compare July 27, 2022 00:26

jcouv added the Feature - Ref Fields label Jul 29, 2022

jcouv force-pushed the runtime-flag branch from fb5fcda to c2a7b58 Compare August 2, 2022 05:26

Ref fields feature requires ByRefFields runtime flag

8b8c0ed

jcouv force-pushed the runtime-flag branch from c2a7b58 to 8b8c0ed Compare August 2, 2022 15:57

jcouv marked this pull request as ready for review August 2, 2022 15:57

jcouv requested a review from a team as a code owner August 2, 2022 15:57

333fred reviewed Aug 2, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/MissingCorLibrarySymbol.cs Show resolved Hide resolved

333fred reviewed Aug 2, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/MissingCorLibrarySymbol.cs Show resolved Hide resolved

333fred reviewed Aug 2, 2022

View reviewed changes

jcouv requested a review from 333fred August 2, 2022 16:57

Skip an EE test too

ceb0f11

cston reviewed Aug 2, 2022

View reviewed changes

Julien Couvreur added 2 commits August 2, 2022 21:20

Remove MscorlibRefWithoutSharingCachedSymbols

d9c5df4

remove unused parameters

618c505

jcouv requested a review from cston August 3, 2022 04:25

cston reviewed Aug 3, 2022

View reviewed changes

Refactor into CreateCompilation

d3f6fec

cston approved these changes Aug 3, 2022

View reviewed changes

Julien Couvreur added 4 commits August 2, 2022 22:18

Rename and factor back a bit

88a43dd

Factor with TestBase

a691536

Merge remote-tracking branch 'dotnet/main' into runtime-flag

0d241dc

merge

09726ea

cston approved these changes Aug 3, 2022

View reviewed changes

333fred reviewed Aug 4, 2022

View reviewed changes

Address feedback from Fred

445b2be

This was referenced Aug 4, 2022

Review usage of CopyWithoutSharingCachedSymbols() in ref field tests #63222

Open

Design a more general test hook solution for runtime feature flags #63223

Closed

cston approved these changes Aug 4, 2022

View reviewed changes

333fred approved these changes Aug 4, 2022

View reviewed changes

Julien Couvreur added 2 commits August 4, 2022 17:37

Put skipExtraValidation: true back

3e564e8

Put skipExtraValidation back in couple tests

03d7207

jcouv merged commit ab3fe7a into dotnet:main Aug 5, 2022

jcouv deleted the runtime-flag branch August 5, 2022 15:01

ghost added this to the Next milestone Aug 5, 2022

allisonchou mentioned this pull request Aug 9, 2022

[Automated] PRs inserted in VS build main-32808.858 #63272

Closed

dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022


		private static string IncludeExpectedOutput(string expectedOutput) => IsNet70OrGreater() ? expectedOutput : null;

		private static MetadataReference MscorlibRefWithoutSharingCachedSymbols


		private static string IncludeExpectedOutput(string expectedOutput) => IsNet70OrGreater() ? expectedOutput : null;

		private static IEnumerable<MetadataReference> CopyWithoutSharingCachedSymbols(IEnumerable<MetadataReference> references)

Conversation

jcouv commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred Aug 2, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

cston Aug 2, 2022 • 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!

cston Aug 2, 2022 • 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!

cston Aug 3, 2022 • 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!

cston Aug 3, 2022 • 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 commented Aug 3, 2022

Uh oh!

jcouv commented Aug 4, 2022

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Aug 4, 2022 • 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!

333fred Aug 4, 2022 • 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!

333fred Aug 4, 2022 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv commented Jul 26, 2022 •

edited

Loading

333fred Aug 2, 2022 •

edited by jcouv

Loading

cston Aug 2, 2022 •

edited by jcouv

Loading

cston Aug 2, 2022 •

edited by jcouv

Loading

cston Aug 3, 2022 •

edited by jcouv

Loading

cston Aug 3, 2022 •

edited

Loading

333fred Aug 4, 2022 •

edited by jcouv

Loading

333fred Aug 4, 2022 •

edited by jcouv

Loading

333fred Aug 4, 2022 •

edited by jcouv

Loading