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

[WIP] Add Reference Assembly support #11521

Closed
wants to merge 82 commits into from
Closed

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented May 4, 2021

This adds the ability for the compiler to emit reference assemblies. See here for more info about reference assemblies; it has the rules.

Adds the compiler options, --refonly and --refout:<file>.

For the F# compiler, there are some unique quirks about generating reference assemblies:

  • Two types of reference assemblies:
    1. Reference assembly with optimization data (requires running the optimizer on impl files)
      • Used for actual compiler input when building a project.
      • This is required because of inline functions that get serialized in F# metadata.
      • This is what the compiler option, --refonly and --refout will do.
    2. Reference assembly without optimization data (does not require running the optimizer)
      • Used for tooling scenarios, editors, IDEs, etc.
      • For impl files with backing sig files, it may be possible to not type-check the impl file and instead produce an impl file that matches the signature.
  • Static linking:
    • You should not be allowed to static link a reference assembly, the compiler should give an error.
  • FSI:
    • Reference assembly generation should not be allowed for FSI; should give an error.

The following are three parts of the reference assembly work, where the first part is the current PR:

Acceptance Criteria Part 1

  • Add performance tests for emitting two assemblies at compile time showing - we expect the time to emit a reference assembly to be very fast and may even consider emitting in parallel to the real assembly.
  • Add tests for --refonly
  • Add tests for --refout
  • Generate MVID for reference assemblies based on contents of the assembly

Acceptance Criteria Part 2

  • Add FSharp build logic to handle and take advantage of reference assemblies. i.e skip compilations if reference assemblies have not changed.

Acceptance Criteria Part 3

  • Fix parameter names for auto-generated equality and comparison methods on reference assembly emission for tooling
  • Add tests for reference assembly emission for tooling

// When emitting a reference assembly, do not emit methods that are private unless they are virtual/abstract or provide an explicit interface implementation.
// Internal methods can be omitted only if the assembly does not contain a System.Runtime.CompilerServices.InternalsVisibleToAttribute.
if cenv.opts.referenceAssemblyOnly &&
(v.Accessibility.IsPrivate || (v.Accessibility.IsInternal && not cenv.hasInternalsVisibleToAttr)) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

The Accessibility thing on Val and Entity is not complete unfortunately and most code relying on it alone is probably buggy. See #11528

Copy link
Contributor Author

@TIHan TIHan May 5, 2021

Choose a reason for hiding this comment

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

We'll be using ILMemberAccess here instead as it's the most accurate, so we don't have to do any extra work for accessibility.

The only downside is that the compiler will always emit Internal for everything and never private. I'm wondering if we could change that in the future. Logically speaking, there are things that really should be emit ILMemberAccess.Private such as backing fields.

TIHan added 3 commits June 4, 2021 11:01
quick test to see if determinism CI breaks when deterministic flag is off, it should
@charlesroddie
Copy link
Contributor

Fixes #3066

@pbiggar
Copy link

pbiggar commented Aug 4, 2021

Super excited for this to land! Do you think it'll make it into the .NET 6 release?

@TIHan TIHan mentioned this pull request Aug 16, 2021
5 tasks
@@ -2,8 +2,7 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFrameworks Condition="'$(OS)' != 'Unix'">net472;net5.0</TargetFrameworks>
<TargetFrameworks Condition="'$(OS)' == 'Unix'">net5.0</TargetFrameworks>
<TargetFrameworks>net472;net5.0</TargetFrameworks>
Copy link
Member

@vzarytovskii vzarytovskii Aug 31, 2021

Choose a reason for hiding this comment

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

I think, this would fail to build on Linux/macOS, since net472 is not supported there. Unintentional change (merge)?

Copy link
Member

Choose a reason for hiding this comment

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

this doesn't have to be the case, if the .net framework reference assemblies nuget package was added as a dependency then these projects could at least build on non-windows.

@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks Condition="'$(OS)' != 'Unix'">net472;net5.0</TargetFrameworks>
<TargetFrameworks>net472;net5.0</TargetFrameworks>
Copy link
Member

@vzarytovskii vzarytovskii Aug 31, 2021

Choose a reason for hiding this comment

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

Probably, unintentional change? This would fail to build on Linux/macOS, since net472 is not supported there.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 25, 2021

I need to fix the test failures and merge conflicts. There is still a few more work items that need to be done here but for the most part I think this can be ready quickly if we scope out producing an assembly for design-time/IDE scenarios.

@TIHan
Copy link
Contributor Author

TIHan commented Nov 4, 2021

Closing in favor of: #12334

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

Successfully merging this pull request may close these issues.

6 participants