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

Add Reference Assembly support - 'refonly' and 'refout' compiler options #12334

Merged
merged 131 commits into from
Apr 20, 2022

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Nov 4, 2021

Update (Vlad):
This PR is scoped to compiler changes only (i.e. --refonly and --refout flags) for producing reference assemblies. Changes related to FSharp.Build (sigdata, optdata and MVID generation) will be part of separate PR(s).

Summary:

General rules for emitting reference assemblies now are:
(note, general behaviour follows what Roslyn does)

  1. Types are always emitted (even if they have no members).

  2. Internal/Private fields are emitted if:

    a. They are part of a struct type (because of "has default value" and "satisfies unmanaged constraint" semantics).
    b. They are part of attribute specification.

  3. Internal/Private properties and their corresponding get_/set_ methods are emitted if:

    a. They are part of attribute specification.

  4. Events are emitted if they have corresponding add_/remove_ methods.

  5. We always skip private MethodDefs and skip internal ones if there's no InternalsVisibleTo attribute.

  6. We always emit throw null for any emitted method bodies.

Description: This PR is part of splitting https://github.com//pull/11521 - as we are trying to reduce the scope of the work. This PR is only meant for compiler output; not IDE support using metadata-only assemblies.

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:

  • 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.
  • 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.

User Benefits

The idea of the compiler emitting reference assemblies is to have faster solution build times when a small change has been made to a source file in a project that doesn't impact the overall signature.

PR Dependencies

Acceptance Criteria

  • Critical Generate deterministic MVID for reference assemblies based on contents of the assembly where the content also includes signature and optimization data; basically SHA1 of the bytes of the entire assembly.
  • Critical Add or modify FSharp.Build tasks that will determine if projects need to be re-built or not depending on if the reference assembly output has changed. See Roslyn's implementation of this: https://github.com/dotnet/roslyn/blob/315c2e149ba7889b0937d872274c33fcbfe9af5f/src/Compilers/Core/MSBuildTask/CopyRefAssembly.cs
  • Critical Add tests to verify determinism when emitting a reference assembly.
    • Should include a test that verifies determinism when a source file has changed in order to ensure we will gain the user benefits mentioned above.
  • Implement --refonly and --refout compiler options.
  • Emit assembly-level attribute ReferenceAssembly for reference assemblies.
  • When emitting a reference assembly, only emit IL instructions ldnull; throw for method bodies that have implementations.
  • When emitting a reference assembly, disable emitting methods that are private unless they are virtual/abstract or provide an explicit interface implementation
  • When emitting a reference assembly, disable emitting internal methods if no InternalsVisibleTo attributes are found in the assembly, except if they are virtual/abstract or provide an explicit interface implementation which is similar to private methods.
  • 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.
  • Add tests that check for an error if static linking is used with a reference assembly.
  • Add tests that check for an error if FSI is used with a reference assembly.

@@ -2122,10 +2122,11 @@ type ILTypeDef(name: string, attributes: TypeAttributes, layout: ILTypeDefLayout
member _.MethodImpls = methodImpls
member _.Events = events
member _.Properties = properties
member _.IsAttribute = isAttribute
Copy link
Contributor Author

@TIHan TIHan Feb 21, 2022

Choose a reason for hiding this comment

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

Storing isAttribute: bool might be a bit heavy weight for ILTypeDef as the question of "is the type an attribute" is answered before construction. Are we not able to determine this by looking at extends?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it, looking at extends won't give you all the information of the hierarchy. Seems the only reliable way of determining if it's an attribute is computing it before creating ILTypeDef. So, probably storing isAttribute: bool is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just note that ILTypeDef tries to store data that is practically 1:1 with assembly metadata, which isAttribute: bool isn't a 1:1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because IsAttribute is only determined before writing an assembly, it does feel odd to have it on ILTypeDef as the type is for both reading and writing.

As a possible alternative, we could build some kind of storage in IlxGen that only hold the identities of types that are attributes. Then we can pass that storage to ilwrite in its cenv. That way those checks would look something like this:
if cenv.isAttrStorage.IsAttribute(td) ...

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about it, looking at extends won't give you all the information of the hierarchy.

Yeah, that was a main reason of computing it for ILTypeDef.

Because IsAttribute is only determined before writing an assembly, it does feel odd to have it on ILTypeDef as the type is for both reading and writing.

As a possible alternative, we could build some kind of storage in IlxGen that only hold the identities of types that are attributes. Then we can pass that storage to ilwrite in its cenv. That way those checks would look something like this: if cenv.isAttrStorage.IsAttribute(td) ...

Let me experiment with.

@@ -1719,6 +1719,7 @@ and typeDefReader ctxtH: ILTypeDefStored =
methodImpls=mimpls,
events= events,
properties=props,
isAttribute=false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like IsAttribute could only be set to true in IlxGen and only used for writing an assembly. This means every read ILTypeDef will have IsAttribute return false. We should probably add a comment to ILTypeDef.IsAttribute describing that it's only used for writing an assembly.

Copy link
Contributor

Choose a reason for hiding this comment

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

This means every read ILTypeDef will have IsAttribute return false.

Using a name like "IsDefinitelyAttribute" or "IsKnownToBeAttribute" may be clarifying

@dsyme
Copy link
Contributor

dsyme commented Mar 1, 2022

@vzarytovskii Needs a merge :)

@vzarytovskii
Copy link
Member

@vzarytovskii Needs a merge :)

Yeah, waiting until main is fixed, should be done after #12769

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Generally looks Okay, please take care of Will's and Don's suggestions before merging.

Thanks

@@ -1168,6 +1170,8 @@ fscTooManyErrors,"Exiting - too many errors"
2026,fscDeterministicDebugRequiresPortablePdb,"Deterministic builds only support portable PDBs (--debug:portable or --debug:embedded)"
2027,fscPathMapDebugRequiresPortablePdb,"--pathmap can only be used with portable PDBs (--debug:portable or --debug:embedded)"
2028,optsInvalidPathMapFormat,"Invalid path map. Mappings must be comma separated and of the format 'path=sourcePath'"
2029,optsInvalidRefOut,"Invalid reference assembly path'"
2030,optsInvalidRefAssembly,"Invalid use of emitting a reference assembly. Check the compiler options to not specify static linking, or using '--refonly' and '--refout' together."
Copy link
Member

Choose a reason for hiding this comment

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

@vzarytovskii
This should read something like:

Invalid use of emitting a reference assembly, do not use --staticlink, or '--refonly and --refout' together.

@vzarytovskii
Copy link
Member

The rest of work is tracked in #13222 and #13223

marcin-krystianc added a commit to marcin-krystianc/sdk that referenced this pull request Jul 1, 2022
Generation of reference assemblies for F# was disabled previously (dotnet#13085) because the F# compiler didn't support them.
Now both, the F# compiler (dotnet/fsharp#12334) and the FCS task (dotnet/fsharp#13263) support reference assemblies. Also, the bug around the copying of F# ref assemblies was fixed (dotnet/fsharp#13266) so it is time to enable the generation of ref assemblies for F#.
marcin-krystianc pushed a commit to marcin-krystianc/sdk that referenced this pull request Jul 1, 2022
Generation of reference assemblies for F# was disabled previously (dotnet#13085) because the F# compiler didn't support them.
Now both, the F# compiler (dotnet/fsharp#12334) and the FCS task (dotnet/fsharp#13263) support reference assemblies. Also, the bug around the copying of F# ref assemblies was fixed (dotnet/fsharp#13266) so it is time to enable the generation of ref assemblies for F#.
marcin-krystianc added a commit to marcin-krystianc/sdk that referenced this pull request Jul 1, 2022
Generation of reference assemblies for F# was disabled previously (dotnet#13085) because the F# compiler didn't support them.
Now both, the F# compiler (dotnet/fsharp#12334) and the FCS task (dotnet/fsharp#13263) support reference assemblies. Also, the bug around the copying of F# ref assemblies was fixed (dotnet/fsharp#13266) so it is time to enable the generation of ref assemblies for F#.s
charlesroddie pushed a commit to charlesroddie/fsharp that referenced this pull request May 2, 2023
…ons (dotnet#12334)

* 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

* 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

* Added some todos + have more readable canGenMethodDef

* Add some more tests

* [WIP]: ignore properties if we don't have getter/setter, or we don't have methoddef for them

* Don't generate private types, generate nested internal types only if the IVT is set

* Merge fix

* Another fix after merge + added more internal tests

* Fixed test framework after merge (output directory). Add check whether we can generate fields (based on IVT and access). Fixed IVT attribute check (check in manifest). Disabled some tests temporary/

* Emit fields when the type is struct. Always emit types

* WIP: added isAttribute to ILTypeRef if type extends Attribute

* Fix properties generation, fix generating getter/setter for attributes

* Only check properties to generate if we are emitting reference assembly

* Fixed surface area tests

* Adjusted baselines for IL tests. Fixed events generation.

* Cleanup unused yaml files

* Fixed docs for ILMemberAccess

* Update message + rename property for ILTypeDef to be more clear

* Surface area tests

* Fixed baseline error message

* After-merge fixes

* Fix tests

Co-authored-by: Vlad Zarytovskii <[email protected]>
Co-authored-by: Don Syme <[email protected]>
@kerams
Copy link
Contributor

kerams commented Oct 26, 2023

Types are always emitted (even if they have no members).

I know Roslyn does this too, but why? Neither https://blog.paranoidcoding.org/2016/02/15/are-private-members-api-surface.html nor https://github.com/dotnet/roslyn/blob/main/docs/features/refout.md expand on that point.

You can surface non-public types by using them in attributes:

type private Y = interface end

type C (_t: Type) =
    inherit Attribute ()

[<C(typeof<Y>)>]
type X = interface end

While removing Y could make an observable difference as far as the API is concerned, I don't see how this affects the compilation of code that uses X. Could we, at the very least, remove internal compiler-generated types like state machine structs, static initializers and closures? In my quick spike I was able to reduce reference assembly sizes by almost 50% by pruning private and internal IL types (when IVT is not involved), without even touching sig and opt data.

@vzarytovskii
Copy link
Member

I know Roslyn does this too, but why?

I don't exactly remember why now. I can dig into my notes it if you'd like to (they're on my old laptop though, so might take some time).

@vzarytovskii
Copy link
Member

vzarytovskii commented Oct 26, 2023

So, C#, apparently, doesn't trim any types from the reference assembly, not even closure types which are resulting from trimmed method implementations. Public fields of private types are not removed, including implementation details such as the fields of closure types, or the static field used to save the singleton delegate for a closure such as (new Func<int, int>(X => X + 1)).

@jaredpar do you know the reason for that? Is it for constraint resolutions?

Struct types (and their fields) shouldn't probably be touched, since they are used in constraints (has default value, unmanaged constraints).

Attributes are an interesting case. Consider an attribute like this:

[AttributeUsage(AttributeTargets.All)]
internal class FooAttribute : Attribute
{
   internal int InternalFieldInInternalClass;
}

C# can't use it as an attribute, F# can use the equivalent type as an attribute.

C#, in this case, can skip emitting internal fields of classes from the reference assembly. F#, it wouldn't be valid to do this for attribute types unless we made a change to the language spec.

@kerams
Copy link
Contributor

kerams commented Oct 26, 2023

I'm not bothered by always keeping internal/private attribute classes - they are so rare compared to other types that dropping them would make little difference.

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.

8 participants