Skip to content
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.

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

Verify synthesized RequiresLocationAttribute cannot be accessed by fnptrs #69328

Merged
merged 7 commits into from
Aug 11, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Aug 2, 2023

Test plan: #68056

@jjonescz jjonescz force-pushed the RefReadonly-24-AttributeEmitTests branch from 34aeaba to a745c27 Compare August 2, 2023 09:19
@jjonescz jjonescz requested a review from jcouv August 2, 2023 09:19
Copy link

@hamarb123 hamarb123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly lgtm and addresses my concerns :)

@jjonescz jjonescz marked this pull request as ready for review August 2, 2023 12:12
@jjonescz jjonescz requested a review from a team as a code owner August 2, 2023 12:12
@jjonescz jjonescz force-pushed the RefReadonly-24-AttributeEmitTests branch from c4b8474 to 2bddd9a Compare August 2, 2023 15:16
@jjonescz
Copy link
Member Author

jjonescz commented Aug 9, 2023

@jcouv PTAL, this is a test-only change

public void M2(ref readonly int p) { }
}
""";
var comp = CreateCompilation(source, options: TestOptions.UnsafeDebugDll);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core CreateCompilation helper is expected to roll forward to the latest target framework as we move forward. At the moment this references the net7.0 assemblies but will move to net8.0 (likely early 2024). At that point the test will fail because this will succeed since RequiresLocationAttribute will be available. Think this test should specify TargetFramework.Net70 explicitly to avoid that future issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that right? Seems that the default target framework is netstandard2.0. Otherwise, I would probably need to change a lot of tests in this file because for example all function pointer tests include the attribute definition as source.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake. Your correct this defaults to TargteFramework.Standard not TargetFramework.Latest.

public void M(ref readonly int p) { }
}
""";
var comp1 = CreateCompilation(source1, assemblyName: "Assembly1").VerifyDiagnostics();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar feedback about all the CreateCompilation uses in this particular PR.

@jaredpar jaredpar self-assigned this Aug 9, 2023
@jjonescz jjonescz requested a review from jaredpar August 10, 2023 08:31
@jjonescz jjonescz merged commit b735c6e into dotnet:main Aug 11, 2023
25 checks passed
@jjonescz jjonescz deleted the RefReadonly-24-AttributeEmitTests branch August 11, 2023 06:16
@ghost ghost added this to the Next milestone Aug 11, 2023
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers New Feature - Ref Readonly Parameters `ref readonly` parameters untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants