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

Fix17713 - Reverting PR - 17649 - Make the interaction between #line and #nowarn directives consistent #17724

Merged
merged 9 commits into from
Sep 16, 2024

Conversation

KevinRansom
Copy link
Member

This language feature caused bootstrapping issues for this repo introducing additional unintended warnings when building the F# bootstrapper with F# 9.0.

This was first noticed by the sourcebuildteam who have a strict bootstrapping requirement for delivery to Linux distributions.

Additionally the langversion:8 implementation produced different error output from the 8.0.* version of the compiler.

The way that we use #line and # n filename in the product is quite extensive and fragile. It is clear that we need a more robust solution highlighted by the issue the original PR was intended to solve, this has highlighted that we do need to address the consistency issue. Everyone on the team agreed that the current implementation that we are going back to is not ideal, but this reversion allows us to identify a better design for a fix.

The warnings by bootstrapping with F# 9.0 produced are:

##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(35,39): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_decl has been constrained to be type 'ParserSpec -> ParserSpec'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(38,107): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_idents has been constrained to be type 'Identifier list'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(38,107): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_idents has been constrained to be type 'Identifier list'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(46,42): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_rule has been constrained to be type 'Identifier * Rule list'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(46,42): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_rule has been constrained to be type 'Identifier * Rule list'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(47,69): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_clauses has been constrained to be type 'Rule list'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(47,69): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_clauses has been constrained to be type 'Rule list'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fs(411,20): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_optbar has been constrained to be type 'unit'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fs(411,20): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_optbar has been constrained to be type 'unit'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fs(431,20): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_optsemi has been constrained to be type 'unit'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fs(431,20): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_optsemi has been constrained to be type 'unit'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(50,51): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_clause has been constrained to be type 'Rule'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(50,51): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_clause has been constrained to be type 'Rule'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(51,55): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_syms has been constrained to be type 'Identifier list'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(51,55): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_syms has been constrained to be type 'Identifier list'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(51,58): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_optprec has been constrained to be type 'Identifier option'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fsy(51,58): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype_optprec has been constrained to be type 'Identifier option'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]
 /vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fs(223,20): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype__startspec has been constrained to be type 'obj'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj] [/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj]
##[error]/vmr/src/fsharp/buildtools/fsyacc/fsyaccpars.fs(223,20): error FS0064: This construct causes code to be less generic than indicated by the type annotations. The type variable 'gentype__startspec has been constrained to be type 'obj'. [/vmr/src/fsharp/buildtools/fsyacc/fsyacc.fsproj]

@KevinRansom KevinRansom requested a review from a team as a code owner September 13, 2024 16:48
Copy link
Contributor

github-actions bot commented Sep 13, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@vzarytovskii
Copy link
Member

FYI @Martin521, it was blocking our sourcebuild pipeline (Kevin tried to fix it yesterday, but no luck apparently).

@KevinRansom KevinRansom added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Sep 13, 2024
@KevinRansom KevinRansom enabled auto-merge (squash) September 14, 2024 01:01
@Martin521
Copy link
Contributor

Martin521 commented Sep 15, 2024

FYI @Martin521, it was blocking our sourcebuild pipeline (Kevin tried to fix it yesterday, but no luck apparently).

How can I reproduce this?
I am not sure what "bootstrapping with F# 9.0" exactly means.
It might make sense to integrate at least the changes to fsyaccdriver.fs and the *.fsy files to avoid the same problem next year.

@KevinRansom
Copy link
Member Author

@Martin521 Build the bootstrapper with an F# 9.0 compiler with these changes.

@Martin521
Copy link
Contributor

Martin521 commented Sep 16, 2024

Ok, if that is a requirement, then this fix can never be done in one step.
It is important then, to already now insert the extra #nowarn directives into the grammar files.
This doesn't hurt, does not change the behavior or output, but enables us next year to create a fix.

EDIT: It can be done in one step, with just one additional #nowarn in fsyaccpars. See below.

Martin521

This comment was marked as outdated.

@Martin521
Copy link
Contributor

Martin521 commented Sep 16, 2024

#nowarn "64" should be provisionally inserted in fsyaccpars.fs after line 15.
I will create a PR in the FsLexYacc repository to change the upstream source fsyaccpars.fsy to the same effect.

@Martin521
Copy link
Contributor

Martin521 commented Sep 16, 2024

Actually, with just this additional #nowarn in fsyaccpars, the original fix (#17649) will work also in F# 9 bootstrapping.
So, up to the team if the fix should happen now or next year.

@KevinRansom
Copy link
Member Author

@Martin521 we are going with the reversion for now, we need to have a fuller discussion on how we want nowarn to work if we decide to change it.

Here is my discussion of why I believe reversion is the way to go right now: #17748 (comment)

@Martin521
Copy link
Contributor

The "scoped nowarn" RFC discussion thread might be an appropriate place for a fuller discussion.

@vzarytovskii
Copy link
Member

The "scoped nowarn" RFC discussion thread might be an appropriate place for a fuller discussion.

Yeah, we should continue there. @KevinRansom @T-Gro we should discuss it on some of the upcoming triage sessions

@KevinRansom KevinRansom merged commit 848c038 into dotnet:main Sep 16, 2024
32 checks passed
@ellahathaway
Copy link
Member

Please make sure that this flows into .NET 9.0. I believe fsharp's refs/heads/release/dev17.12 branch flows to .NET 9.0 release branch. Thanks in advance :)

@ellahathaway
Copy link
Member

Please make sure that this flows into .NET 9.0. I believe fsharp's refs/heads/release/dev17.12 branch flows to .NET 9.0 release branch. Thanks in advance :)

@dotnet/fsharp - This change needs to be ported into .NET 9. What is the best way to get this backported to the fsharp's refs/heads/release/dev17.12 branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants