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

Fix #17713 (sourcebuild phase 2 build error) #17748

Closed
wants to merge 2 commits into from

Conversation

Martin521
Copy link
Contributor

Description

This is a fix for #17713, by a change on top of #17649 to make the interaction between #line and #nowarn directives consistent.
This PR is an alternative to #17724 (reverting #17649).

It updates fsyaccpars.fs to the latest version of the upstream repository.

@Martin521 Martin521 requested a review from a team as a code owner September 16, 2024 11:43
Copy link
Contributor

✅ No release notes required

@ellahathaway
Copy link
Member

Please also backport this to 9.0 (I believe fsharp's refs/heads/release/dev17.12 branch flows to .NET 9.0 release branch). Thanks in advance :)

@KevinRansom
Copy link
Member

KevinRansom commented Sep 16, 2024

I'm afraid this isn't the right fix:

Firstly,
Modifying lex and yacc in this repo is not permitted. Although the project file is modifiable. It is fair to say that modifications have happened, but they actually shouldn't have been and work will have to be done at some time to re-align with the upstream lex and yacc repo, or we change our mind on ingesting the features. My vote is on ingesting lex and yacc, but others have firmer opinions than I.
readme describing why here: https://github.com/dotnet/fsharp/blob/main/buildtools/README-fslexyacc.md

Secondly,
The language version 8.0 flag introduces behavior changes that were not present in earlier versions of the compiler:
Consider:
image

When compiled with 8.0.400
image

Using ConsistentNowarnLineDirectiveInteraction when compiled with /langversion:8.0
image

Note!!!!: The purpose of the langversion switch is to select the language version that is compatible with earlier versions of the compiler. The intent is to support teams of developers with mixed toolsets, I.e one dev with a net8.0 toolset and another with net9.0 --- the net 9.0 developer selects langversion 8.0 so that he doesn't use new features or introduce new errors that will impact the developer using the net 8.0 toolset.

Thirdly,
once the sourcebuild issue popped up we had spirited discussions about how the feature should actually behave. As such we are not comfortable taking this change at this time.

I do agree that the current implementation in the shipped compilers is not great and needs to be redesigned and re-implemented. However, the change proposed in the ConsistentNowarnLineDirectiveInteraction may not be the correct one.

@Martin521
Copy link
Contributor Author

On the first item:
As mentioned in the description of this PR, the change was actually done in the upstream fslexyacc repo and copied back here.
So, everything done as required.

The second and third item were described in detail in the original PR.
I would have appreciated comments there and some participation in the "spirited discussions".

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.

3 participants