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

Real accessibility #15484

Merged
merged 71 commits into from Mar 1, 2024
Merged

Real accessibility #15484

merged 71 commits into from Mar 1, 2024

Conversation

KevinRansom
Copy link
Member

@KevinRansom KevinRansom commented Jun 24, 2023

When the F# compile writes out an assembly, it writes all members with internal and private scopes in the source code as internal. All members of types which are internal are written to the assembly with the IL internal scope. This PR introduces a new compiler switch that preserves the source code annotated scope in the IL.

For example the source code:
Module example:
image
Class type example:
image

This change of IL scope impacts the initalization code. Originally intialization required the generation of a type per source file with a .cctor containing all of the initialization code. With real IL signatures this initialization has had to move to a classInitialization method on each class that requires initialization. That is because internal values can be accessed from anywhere inside of the assembly, whereas a private type that is initialized can only happen from a location where that value can be accessed. Ie the class it is specified in and any nested class below it.
In C# the initialization method is the .cctor for that class, unfortunately F# initialization is more structured than C#, C# initialization happens when the type is first used, whereas in F# initialization happens in the order of definition of the values within the source file.

Consider this F# program:
image

It produces output in this order:
image

So the initializers need to be invoked with care.

It should also be noted that a type reference can activate any of these types first, and so doing the initialization solely in class constructors would cause the intiialization sequenced to be out of order.

@KevinRansom KevinRansom requested a review from a team as a code owner June 24, 2023 09:06
@KevinRansom KevinRansom force-pushed the accessibility branch 4 times, most recently from 7b951fc to dd8cd2d Compare July 7, 2023 18:39
@KevinRansom KevinRansom force-pushed the accessibility branch 3 times, most recently from cf916e1 to 8b27fdb Compare July 19, 2023 22:46
@vzarytovskii vzarytovskii marked this pull request as draft July 20, 2023 16:50
@KevinRansom KevinRansom force-pushed the accessibility branch 2 times, most recently from 1c07e1e to a5d2be2 Compare July 21, 2023 07:08
@KevinRansom KevinRansom force-pushed the accessibility branch 5 times, most recently from 22a4bd2 to e9ed9b9 Compare August 4, 2023 08:34
@KevinRansom KevinRansom force-pushed the accessibility branch 6 times, most recently from 23de074 to f714203 Compare August 12, 2023 00:54
@KevinRansom KevinRansom force-pushed the accessibility branch 5 times, most recently from 2af1d80 to bbbbd7e Compare August 21, 2023 02:42
@KevinRansom KevinRansom force-pushed the accessibility branch 3 times, most recently from f9076e2 to 1b4d64e Compare August 31, 2023 03:23
@KevinRansom KevinRansom enabled auto-merge (squash) March 1, 2024 03:14
@KevinRansom KevinRansom merged commit 68a7dae into dotnet:main Mar 1, 2024
31 checks passed
@@ -14,47 +14,104 @@ module SeqExpressionStepping =
|> withNoOptimize
|> withEmbeddedPdb
|> withEmbedAllSource
|> withRealInternalSignatureOn
Copy link
Contributor

Choose a reason for hiding this comment

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

Some tests in this file have withRealInternalSignatureOff, but they still then call verifyCompilation, which will add --realsig+ back, no? Did you mean to remove this from here? @KevinRansom

Copy link
Contributor

Choose a reason for hiding this comment

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

...And wouldn't the alternating tests overwrite each other's baseline files anyway, since there are no separate baseline suffixes specified?

Copy link
Member Author

Choose a reason for hiding this comment

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

...And wouldn't the alternating tests overwrite each other's baseline files anyway, since there are no separate baseline suffixes specified?

@brianrourkeboll
Let me take a look. TBH --- I am now hating all of the IL baseline tests. Just on a general, "I hate them" basis.

Copy link
Member Author

Choose a reason for hiding this comment

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

@brianrourkeboll
You are correct I will fix the tests,
thanks
Kevin

Copy link
Contributor

Choose a reason for hiding this comment

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

@KevinRansom Or I could in my PR (#16650), if that would be easier? Otherwise I'll just have more merge conflicts to resolve lol

Copy link
Member Author

Choose a reason for hiding this comment

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

I got it, here she blows matey: #16795

Copy link
Member Author

Choose a reason for hiding this comment

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

@brianrourkeboll , please look it over and I will get it merged.

psfinaki pushed a commit that referenced this pull request Mar 4, 2024
* Real accessibility (#15484)

* merge

* remove unused binding

* temp

* temp

* temp

* temp

* temp

* temp

* fantomas

* temp

* temp

* quotes

* temp

* realsig build and test

* tuples, staticint tests

* SerializableAttribute tests

* SeqExpressionStepping

* AsyncExpressionStepping

* misc

* AttributeTargets

* CCtorDUWithMember ListExpressionStepping

* temp

* cleanup

* fantomas

* temp

* temp

* temp

* Automated command ran: fantomas

  Co-authored-by: KevinRansom <5175830+KevinRansom@users.noreply.github.com>

* Some cleanup

* clean

* fantoms

* temp

* merge issues

* fantomas

* temp

* Update src/Compiler/TypedTree/TypedTreeBasics.fs

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>

* Update src/Compiler/Optimize/Optimizer.fs

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>

* inline

* Fix plain build.

* Update changelog

* Fixed release notes

* feedback

* remove surplus realsigs

* Update src/Compiler/TypedTree/TypedTree.fsi

Co-authored-by: Petr Pokorny <petr@innit.cz>

* Update tests/FSharp.Compiler.ComponentTests/EmittedIL/ComputationExpressions/ComputationExpressions.fs

Co-authored-by: Petr Pokorny <petr@innit.cz>

* baselines

* baselines

* baselines

* build.sh

* restore quotes

* moar quotes

* mutable police

* fantomas

* t

* Update baselines

* Shadowing/LinqCount.fsx baseline

* Shadowing lingcount

* access.fsx

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
Co-authored-by: Petr Pokorny <petr@innit.cz>

* Fix range start of INTERP_STRING_PART (#16785)

* use the same mechanism we used to fix the range start of INTERP_STRING_END to also fix the range start of INTERP_STRING_PART

* - use a new rule for '"}" +' to catch the correct range start
- clean up work around structures introduced before and not needed anymore with this

* Revert "- use a new rule for '"}" +' to catch the correct range start"

This reverts commit 4d01cda.

* add second PR to changelog

* remove commented poc code

* Enforce union case declarations AttributeTargets (#16764)

* Enforce union-cases AttributeTargets

* release notes

* LanguageFeature.EnforceAttributeTargetsUnionCaseDeclarations

* release notes

* format code

* improve naming

* Update src/Compiler/Checking/CheckExpressions.fs

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Fix merge conflict

---------

Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>

* Don't consider parse warnings as errors in ComputeTcIntermediate (#16792)

* Fix seqexpression testcases (#16795)

* correct realsignature test cases for seqexpr tests

* rename typo

---------

Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
Co-authored-by: Petr Pokorny <petr@innit.cz>
Co-authored-by: dawe <dawedawe@posteo.de>
Co-authored-by: Edgar Gonzalez <edgar.gonzalez@fundourselves.com>
Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com>
Co-authored-by: Florian Verdonck <florian.verdonck@outlook.com>
@auduchinok
Copy link
Member

The change sounds great!

This PR introduces a new compiler switch that preserves the source code annotated scope in the IL.

Is this going to be a new default one day?

@vzarytovskii
Copy link
Member

The change sounds great!

This PR introduces a new compiler switch that preserves the source code annotated scope in the IL.

Is this going to be a new default one day?

Maybe one day, if stars align.

@auduchinok
Copy link
Member

Maybe one day, if stars align.

With so much work done here, it would be sad to have it hidden under a feature flag 🙂
Not enabling it also means we'd have much less real life testing, compared to what the most of the users would use.

@vzarytovskii
Copy link
Member

Maybe one day, if stars align.

With so much work done here, it would be sad to have it hidden under a feature flag 🙂 Not enabling it also means we'd have much less real life testing, compared to what the most of the users would use.

Sure, as it is with all opt-in features and warnings. It's a huge change in codegen and takes time.

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.

None yet

8 participants