Recognize IsByRefLikeAttribute from user code. #11374

teo-tsirpanis · 2021-04-04T10:16:51Z

This PR adds support to define ref structs on assemblies targeting frameworks that do not have the System.Runtime.CompilerServices.IsByRefLikeAttribute defined, namely .NET Standard 2.0 and .NET Framework versions earlier than 4.7.1, by recognizing the attribute defined by the users themselves.

The logic of isByrefLikeTyconRef is updated to find the attribute by name, instead of finding a specific attribute type on a system assembly. Consequently, the TcGlobals.attrib_IsByRefLikeAttribute property was removed and replaced by a tname_IsByRefLikeAttribute constant containing the attribute's full name, and code trying to find that specific attribute was updated to call isByrefLikeTyconRef, benefiting from caching the result of the lookup.

~~A check that the attribute is applied on non-struct types was moved from the post-inference checks to the isByrefLikeTyconRef function to consolidate the attribute's existence check.~~ Reverted

I was going to do the same with the IsReadOnlyAttribute but it was more complicated since it might get emitted (not just checked), and such deficiency has almost zero impact.

I will also need some help on testing this change.

TIHan · 2021-05-06T15:51:54Z

It does make sense to check for the name. C# compiler will emit its own IsByRefLikeAttribute if it can't find it in the referenced assemblies when trying to declare a ref-struct.

For F#, since it doesn't use a keyword, adding the attribute ourselves and trying to use it makes sense.

teo-tsirpanis · 2021-05-23T22:30:24Z

It now (almost) works, could somebody review it?

There is a test that defines a ref struct with a custom attribute and assumes the compiler will not treat it specially. What to do with that? Another test that assumes the previous behavior is affected as well.

And I would appreciate some help on what kinds of tests to add and how.

teo-tsirpanis · 2021-09-24T10:05:13Z

@dsyme (I don't know who to tag) could you please take a look?

dsyme · 2021-09-24T16:10:23Z

@teo-tsirpanis

Your code looks fine, just adjust the tests to get it green

There is a test that defines a ref struct with a custom attribute and assumes the compiler will not treat it specially. What to do with that?

I guess change the thing the test checks for so that the new behaviour is noted

Another test that assumes the previous behavior is affected as well.

Delete this test I guess or at least remove the part of it that's relevant

teo-tsirpanis · 2021-09-25T13:49:20Z

@dsyme, phew, the tests are now finally green, and I added another one. 😌 Thanks for looking at it.

src/fsharp/TypedTreeOps.fs

src/fsharp/AugmentWithHashCompare.fs

src/fsharp/TcGlobals.fs

src/fsharp/TypedTreeOps.fs

teo-tsirpanis · 2021-10-06T13:35:18Z

Is everything OK with this PR? I would love to see it landed in in time for .NET 6.

dsyme · 2021-10-06T14:24:56Z

I'm ok with this PR but will leave @TIHan to approve. We need to decide if it goes under a language feature flag

@teo-tsirpanis The cutoff is past for feature inclusion for .NET 6 (some bug fixes are ok). We can put this on the train for the next release after.

baronfel · 2021-10-06T14:25:51Z

@dsyme clarifying question here: 'next release after' means the 6.1.2xx train, not waiting for the 7.x.1xx releases?

dsyme · 2021-10-06T14:26:51Z

@dsyme clarifying question here: 'next release after' means the 6.1.2xx train?

I'd imagine so though I don't specifically know - F# bits ship in both .NET SDK and VS and @brettfo might be able to say

teo-tsirpanis · 2021-10-06T15:26:16Z

feature inclusion

I would consider this a bug fix. It does not introduce new functionality, but corrects a wrong behavior. The associated issue is tagged with [Bug] as well.

If it doesn't make it to .NET 6 RTM, waiting until the 6.0.2xx SDK is fine, but a fix on .NET 7 would have been too much for me and will force me to develop temporary workarounds.

brettfo · 2021-10-06T17:39:44Z

@baronfel @dsyme "next release after" should be 6.0.2xx.

TIHan · 2021-10-06T17:49:27Z

We need to decide if it goes under a language feature flag

ByRefLike types were introduced early on, before F# 4.7. We already recognize ByRefLikeAttributes, just not a user-defined version of it. I think this was an oversight on our part and we should consider this a bug fix related to that feature.

tests/fsharp/Compiler/Language/SpanTests.fs

…ework assembly. And use isByrefLikeTyconRef instead of specifically checking whether the attribute is applied. A post-inference check about this attribute was moved to the aforementioned function.

And use tname_IsByRefLikeAttribute without the TcGlobals qualification.

It wasn't holding the namespace's components but the inner types if it was nested. The project now builds.

teo-tsirpanis · 2021-12-01T08:46:39Z

OK @TIHan, I added the test. Can you please take a look?

vzarytovskii · 2021-12-01T11:28:49Z

/azp run

azure-pipelines · 2021-12-01T11:29:11Z

Azure Pipelines successfully started running 2 pipeline(s).

tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests.fsproj

teo-tsirpanis · 2021-12-01T17:37:47Z

All tests are now green.

vzarytovskii · 2021-12-01T20:26:19Z

Thanks, that looks good!

* Adding 'refonly' command line option * Added a simple test, but it needs to fail * We need to emit two kinds of reference assemblies. one with optimizations and ones without * Passing reference assembly flag to IlxGen * Emit ReferenceAssemblyAttribute * Added ref-assembly rules for private and internal methods * use --refonly for now * Use HasFSharpAttribute * Added a failing test * Test passes * Trying to handle anonymous record types * Cleaning up. Using ILMemberAccess instead of Accessibility due to how the compiler understands Accessibility. * Using notlazy * Added another comment * Added mkDummyParameterVal * Using taccessPublic * More cleanup * Minor comment update * more cleanup * Adding FreeAnonRecdTypeInfos set * Adding options * Flowing free anonrecdtypeinfos * Fixing build * Tests pass. Able to emit partial ref assembly with anon recds * Minor rename * Added a failing test * Added failing test * Simpler handling of building lambdas * Trying to figure out default param names * Adding TryEmitReferenceAssembly * Moving some reference assembly generation rules to ilwrite * Fixing build * Added new compiler option '--refout:<file>' * Fixing one of the tests * refonly/refout should only be part of fsc * Updating help baseline * fixed build * Fixing build. Added basic deterministic test * Failing determinism test * Added DeterministicTests * Adding determinism task for CI * moving yml to pipelines * Trying to fix determinism CI * quick fix * removing job * Trying to fix ci * Removing this * Turn on determinism for build * Trying to fix * This works * Determinism * Building * Forgot to run test * Adding job * Trying to fix job * Remove job * Trying to figure out jobs * Updating job * Fixing determinism job * Fixing job * Update test-determinism.ps1 * Update FSharp.Profiles.props quick test to see if determinism CI breaks when deterministic flag is off, it should * Update test-determinism.ps1 * Update FSharpBuild.Directory.Build.props * Trying to fix build * Trying to fix build * fixing build * Fixing build * fixing build * Fixing build * Remove comment as it is not accurate * Removed generating metadata assembly for IDEs * Fixing build * Removing tests * Update ParseAndCheckInputs.fs * Update TypedTree.fs * Fixing build * Update TypedTreeOps.fs * Fixing build * Fixing build * Fixing build * Fixing build * Revert "Added CI job for deterministic builds" (dotnet#12446) This reverts commit a847a71. * Fix adding new opens when namespace declaration is on 1st line (dotnet#12443) * [main] Update dependencies from dotnet/arcade (dotnet#12450) Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Update Microsoft.FSharp.Targets (dotnet#12453) Add `WarningsNotAsErrors` to be forwarded to the compiler * [main] Update dependencies from dotnet/arcade (dotnet#12455) Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Update dependencies from https://github.com/dotnet/arcade build 20211126.2 (dotnet#12459) Microsoft.DotNet.Arcade.Sdk From Version 7.0.0-beta.21574.3 -> To Version 7.0.0-beta.21576.2 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 1487500 (dotnet#12461) * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 1487500 * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 1487500 * Update dependencies from https://github.com/dotnet/arcade build 20211126.4 (dotnet#12462) Microsoft.DotNet.Arcade.Sdk From Version 7.0.0-beta.21576.2 -> To Version 7.0.0-beta.21576.4 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Include range of attributes in SynExceptionDefnRepr, SynExceptionSig & SynModuleSigDecl.Exception. (dotnet#12441) * Disable auto-formatting by default + add an 'Experimental' suffix to the option (dotnet#12454) * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 1489251 (dotnet#12468) * Fix compiler build when the username has a space (dotnet#12472) * Recognize IsByRefLikeAttribute from user code. (dotnet#11374) * Allow IsByRefLikeAttribute to be specified in user code or a non-framework assembly. And use isByrefLikeTyconRef instead of specifically checking whether the attribute is applied. A post-inference check about this attribute was moved to the aforementioned function. * Fix a typo. * Remove some leftover code that was causing errors. And use tname_IsByRefLikeAttribute without the TcGlobals qualification. * Handle the ILTypeSpec.Enclosing property correctly. It wasn't holding the namespace's components but the inner types if it was nested. The project now builds. * Test that custom IsByRefLikeAttributes are recognized. * Fix one failing test. * Fix another failing test. * Address PR feedback: move a check back to PostInferenceChecks.fs. * Add another test. * Remove three redundant package references from FSharp.Core. (dotnet#12479) * Add deterministic builds to the CI (dotnet#12451) Co-authored-by: Will Smith <[email protected]> * Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 1499018 (dotnet#12492) * fixTests (dotnet#12490) * [main] Update dependencies from dotnet/arcade (dotnet#12483) * Update dependencies from https://github.com/dotnet/arcade build 20211202.3 Microsoft.DotNet.Arcade.Sdk From Version 7.0.0-beta.21576.4 -> To Version 7.0.0-beta.21602.3 * Update dependencies from https://github.com/dotnet/arcade build 20211203.6 Microsoft.DotNet.Arcade.Sdk From Version 7.0.0-beta.21576.4 -> To Version 7.0.0-beta.21603.6 * Update dependencies from https://github.com/dotnet/arcade build 20211206.6 Microsoft.DotNet.Arcade.Sdk From Version 7.0.0-beta.21576.4 -> To Version 7.0.0-beta.21606.6 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Update baseline for fcs 'help' test * Added a test for '--refout', with outout and IL verification * Added tests to verify that static linking and refassemblies cannot be used together * Add mvid test for refonly + private members. It is failing on purpose, until MVID generation is fixed * WIP: Add some more to the tests * Added more tests for MVID * wip Co-authored-by: Will Smith <[email protected]> Co-authored-by: Vlad Zarytovskii <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Rodion Ivanov <[email protected]> Co-authored-by: dotnet bot <[email protected]> Co-authored-by: Florian Verdonck <[email protected]> Co-authored-by: Ye <[email protected]> Co-authored-by: Theodore Tsirpanis <[email protected]> Co-authored-by: Kevin Ransom (msft) <[email protected]>

teo-tsirpanis marked this pull request as draft May 2, 2021 16:24

teo-tsirpanis marked this pull request as ready for review May 23, 2021 22:24

KevinRansom self-requested a review June 28, 2021 18:41

teo-tsirpanis force-pushed the isbyreflike-attribute branch 2 times, most recently from 380d841 to a804382 Compare September 25, 2021 12:50

TIHan reviewed Sep 25, 2021

View reviewed changes

src/fsharp/TypedTreeOps.fs Outdated Show resolved Hide resolved

TIHan reviewed Sep 25, 2021

View reviewed changes

src/fsharp/AugmentWithHashCompare.fs Show resolved Hide resolved

TIHan reviewed Sep 25, 2021

View reviewed changes

src/fsharp/TcGlobals.fs Show resolved Hide resolved

TIHan reviewed Sep 25, 2021

View reviewed changes

src/fsharp/TypedTreeOps.fs Show resolved Hide resolved

TIHan reviewed Oct 6, 2021

View reviewed changes

tests/fsharp/Compiler/Language/SpanTests.fs Show resolved Hide resolved

teo-tsirpanis added 8 commits November 30, 2021 23:26

Allow IsByRefLikeAttribute to be specified in user code or a non-fram…

d9d7559

…ework assembly. And use isByrefLikeTyconRef instead of specifically checking whether the attribute is applied. A post-inference check about this attribute was moved to the aforementioned function.

Fix a typo.

f1aa231

Remove some leftover code that was causing errors.

4eb9d52

And use tname_IsByRefLikeAttribute without the TcGlobals qualification.

Handle the ILTypeSpec.Enclosing property correctly.

13b1cda

It wasn't holding the namespace's components but the inner types if it was nested. The project now builds.

Test that custom IsByRefLikeAttributes are recognized.

5a6279b

Fix one failing test.

0db405c

Fix another failing test.

bff7ea2

Address PR feedback: move a check back to PostInferenceChecks.fs.

340d457

teo-tsirpanis force-pushed the isbyreflike-attribute branch from 875771a to 340d457 Compare November 30, 2021 21:30

vzarytovskii closed this Dec 1, 2021

vzarytovskii reopened this Dec 1, 2021

vzarytovskii reviewed Dec 1, 2021

View reviewed changes

tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests.fsproj Outdated Show resolved Hide resolved

vzarytovskii approved these changes Dec 1, 2021

View reviewed changes

teo-tsirpanis force-pushed the isbyreflike-attribute branch from f05f2bd to b756eb1 Compare December 1, 2021 12:16

Add another test.

a4961ec

teo-tsirpanis force-pushed the isbyreflike-attribute branch from b756eb1 to a4961ec Compare December 1, 2021 14:49

vzarytovskii merged commit 6dcf492 into dotnet:main Dec 1, 2021

teo-tsirpanis deleted the isbyreflike-attribute branch December 1, 2021 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recognize IsByRefLikeAttribute from user code. #11374

Recognize IsByRefLikeAttribute from user code. #11374

teo-tsirpanis commented Apr 4, 2021 •

edited

Loading

TIHan commented May 6, 2021

teo-tsirpanis commented May 23, 2021 •

edited

Loading

teo-tsirpanis commented Sep 24, 2021

dsyme commented Sep 24, 2021

teo-tsirpanis commented Sep 25, 2021 •

edited

Loading

teo-tsirpanis commented Oct 6, 2021

dsyme commented Oct 6, 2021

baronfel commented Oct 6, 2021 •

edited

Loading

dsyme commented Oct 6, 2021

teo-tsirpanis commented Oct 6, 2021

brettfo commented Oct 6, 2021

TIHan commented Oct 6, 2021

teo-tsirpanis commented Dec 1, 2021

vzarytovskii commented Dec 1, 2021

azure-pipelines bot commented Dec 1, 2021

teo-tsirpanis commented Dec 1, 2021

vzarytovskii commented Dec 1, 2021

Recognize IsByRefLikeAttribute from user code. #11374

Recognize IsByRefLikeAttribute from user code. #11374

Conversation

teo-tsirpanis commented Apr 4, 2021 • edited Loading

TIHan commented May 6, 2021

teo-tsirpanis commented May 23, 2021 • edited Loading

teo-tsirpanis commented Sep 24, 2021

dsyme commented Sep 24, 2021

teo-tsirpanis commented Sep 25, 2021 • edited Loading

teo-tsirpanis commented Oct 6, 2021

dsyme commented Oct 6, 2021

baronfel commented Oct 6, 2021 • edited Loading

dsyme commented Oct 6, 2021

teo-tsirpanis commented Oct 6, 2021

brettfo commented Oct 6, 2021

TIHan commented Oct 6, 2021

teo-tsirpanis commented Dec 1, 2021

vzarytovskii commented Dec 1, 2021

azure-pipelines bot commented Dec 1, 2021

teo-tsirpanis commented Dec 1, 2021

vzarytovskii commented Dec 1, 2021

teo-tsirpanis commented Apr 4, 2021 •

edited

Loading

teo-tsirpanis commented May 23, 2021 •

edited

Loading

teo-tsirpanis commented Sep 25, 2021 •

edited

Loading

baronfel commented Oct 6, 2021 •

edited

Loading