-
Notifications
You must be signed in to change notification settings - Fork 790
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
Scoped nowarn #18049
Open
Martin521
wants to merge
37
commits into
dotnet:main
Choose a base branch
from
Martin521:scoped-nowarn
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Scoped nowarn #18049
Changes from 28 commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
7498b0b
WarnScopes (1): feature flag, baseline tests, and WarnScopes module
Martin521 9590899
WarnScopes (2): changes to lexing and parsing
Martin521 7b2eeb2
WarnScopes (3): remove legacy #nowarn processing
Martin521 4520e55
WarnScopes (4): integrate the new functionality and add tests
Martin521 0fdaa43
WarnScopes (5): add warn directive trivia
Martin521 fb848d9
WarnScopes (6): enable warn directive trivia
Martin521 e623fde
WarnScopes (7): remove defunct types and parameters
Martin521 62eb32a
WarnScopes (8): IlVerify baseline update
Martin521 5c77ecd
WarnScopes (9): ran fantomas, updated release notes
Martin521 bbdea9d
updated PR number in release notes
Martin521 1c97f60
adapted scipt test to WarnScopes
Martin521 ca6fd7a
updated PR link in release notes (now really)
Martin521 0d0e7bf
Merge remote-tracking branch 'upstream/main' into scoped-nowarn-dev
Martin521 29212c5
skipped a test in vsintegration that might need a vs update
Martin521 6c3ab12
Merge remote-tracking branch 'upstream/main' into scoped-nowarn-dev
Martin521 ceb7e38
Merge remote-tracking branch 'upstream/main' into scoped-nowarn-dev
Martin521 23e366f
merge upstream/main
Martin521 a727e67
moved ilverify baselines
Martin521 cdfef12
Merge remote-tracking branch 'upstream/main' into scoped-nowarn
Martin521 e8a35f4
merge upstream/main
Martin521 2214e83
merge upstream/main
Martin521 5f358b3
update ilverify baselines
Martin521 30d66e2
merge upstream/main
Martin521 387049e
Merge remote-tracking branch 'upstream/main' into scoped-nowarn
Martin521 d617fbd
Merge remote-tracking branch 'upstream/main' into scoped-nowarn
Martin521 e7eb32f
update ilverify baseline (line number changes only)
Martin521 f05f993
merge upstream/main
Martin521 eba854f
merge upstream/main
Martin521 7fa6352
merge upstream/main
Martin521 1301542
fix merge issue
Martin521 48d0875
ilverify baselines
Martin521 2d29b15
Merge remote-tracking branch 'upstream/main' into scoped-nowarn
Martin521 2f44500
made WarnScopes.IsWarnon / IsNowarn public
Martin521 f526549
merge upstream/main
Martin521 7e96882
added surface area change (IsNowarn/IsWarnon) to baseline
Martin521 c278c6a
Merge remote-tracking branch 'upstream/main' into scoped-nowarn
Martin521 bbb8d7c
merge upstream/main
Martin521 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a mutator for langVersion is fine but mostly unnecessary, but please don't let the langVersion mutator set other values in TcConfig that is not the paradigm we use. Yes SetPrimaryAssembly and SetUseSdkRefs also set tcConfigB.fxResolver, they probably shouldn't, actually I think I may fix them so they don't.
TcConfigBuilder is a mutable record and so modifying values is kind of what it's about, after building this is passed to the constructor of TcConfig which then creates the immutable TcConfig that is used to access these values. Perhaps the best thing is to mutate fsharpDiagnosticOptions with the language version when constructing the TcConfig object. Then you would plumb the langversion to the places where FSharpDiagnosticOptions.Default is used and add a langVersion argument to FSharpDiagnosticOptions.Default which would enable the error and warning defaults to change on a per language version basis which is probably usefull longer term. Unfortunately plumbing data about is never fun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
tcConfigB.diagnosticsOptions
is set toFSharpDiagnosticOptions.Default
, the langversion is not yet known. The langversion comes only much later withParseCompilerOpions
.There are two alternatives that I considered but didn't like.
The first was to add to
tcConfigB
an updatedFSharpDiagnosticOptions
(updated with the langversion) in all the places whereParseCompilerOptions
is called (in fsc.fs, fsi.fs, BackgroundCompiler.fs, TransparentCompiler.fs and FSharpCheckerResults.fs).The second was to thread the whole
tcConfig
through all the function calls to the places where I need only the diagnosticsOptions.But let me check if I find still another way.