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

Ad-hoc fix for ref readonly parameters #16232

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

vzarytovskii
Copy link
Member

@vzarytovskii vzarytovskii commented Nov 6, 2023

Fixes dotnet/runtime#94317

Note that this is an ad-hoc fix which will just treat ref readonly as inref during parameters import, for long-term fix, please refer to #16222

ref readonly is encoded as [in]&T with RequiresLocationAttribute, we import inref<_> only from [in]&T with IsReadOnlyAttribute, this PR adds a second check.

@vzarytovskii vzarytovskii marked this pull request as ready for review November 6, 2023 13:27
@vzarytovskii vzarytovskii requested a review from a team as a code owner November 6, 2023 13:27
@vzarytovskii
Copy link
Member Author

That's ready @dotnet/fsharp-team-msft

@baronfel what should we do to backport it to 8.0.1xx which will follow the release? We probably don't want to wait until 8.0.2xx.

If 17.8 maps to 8.0.1xx now in darc, should we just make a back port after release?

@baronfel
Copy link
Member

baronfel commented Nov 6, 2023

Yeah - backport to the branch you have that maps to release/8.0.1xx in the SDK, then get Tactics approval before you merge that PR. Codeflow via darc will take care of the rest and you'll be automatically included in the first servicing release (in December).

@vzarytovskii vzarytovskii merged commit 24ef671 into dotnet:main Nov 7, 2023
24 checks passed
@tannergooding
Copy link
Member

ref readonly is encoded as [in]&T with RequiresLocationAttribute, we import inref<_> only from [in]&T with IsReadOnlyAttribute, this PR adds a second check.

@vzarytovskii, it's worth noting that it while Roslyn currently encodes it as [in]&T, it is not required to have the [in] and only actually looks for &T + RequiresLocation or [IsReadOnly].

It looks like F# isn't actually checking for [in] itself, but wanted to make the callout on what's part of the requirement vs not.

@vzarytovskii
Copy link
Member Author

ref readonly is encoded as [in]&T with RequiresLocationAttribute, we import inref<_> only from [in]&T with IsReadOnlyAttribute, this PR adds a second check.

@vzarytovskii, it's worth noting that it while Roslyn currently encodes it as [in]&T, it is not required to have the [in] and only actually looks for &T + RequiresLocation or [IsReadOnly].

It looks like F# isn't actually checking for [in] itself, but wanted to make the callout on what's part of the requirement vs not.

Yeah, I think we only emit it, not read. As you said, we pretty much only check attributes for refs

@vzarytovskii
Copy link
Member Author

/backport to release/dev17.8

Copy link
Contributor

Started backporting to release/dev17.8: https://github.com/dotnet/fsharp/actions/runs/6826852984

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Use of ref readonly instead of in in libraries is a breaking change for F# callers
5 participants