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.

Sign up for GitHub

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

Jump to bottom

Make nullness equivalent warning message clearer. #18172

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make nullness equivalent warning message clearer. #18172

wants to merge 2 commits into from

Conversation

isaacabraham
Copy link
Contributor

@isaacabraham isaacabraham commented Dec 22, 2024 •
edited
Loading

Description

Fixes #18171. Warning now reads:

Nullness warning: A non-nullable 'String' was expected but this expression is nullable. Consider either changing the target to also be nullable, or use pattern matching to safely handle the null case of this expression.

Checklist

  • Test cases updated

NO_RELEASE_NOTES

@isaacabraham isaacabraham requested a review from a team as a code owner December 22, 2024 15:01
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 22, 2024 •
edited
Loading

⚠️ 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

@isaacabraham
Copy link
Contributor Author

Is this failing because of the lack of release notes or have I botched something else?

Also - what causes the BSL files to get regenerated? I ended up doing it by hand.

@majocha
Copy link
Contributor

majocha commented Dec 23, 2024

@isaacabraham looks like the only error apart from lack of release notes was this crash. If it passes locally it is most probably good.

@smoothdeveloper
Copy link
Contributor

@isaacabraham setting the environment variable TEST_UPDATE_BSL to 1 and running the test should update your .bsl files.

@dotnet dotnet deleted a comment from github-actions bot Dec 23, 2024
@isaacabraham
Copy link
Contributor Author

isaacabraham commented Dec 24, 2024 •
edited
Loading

@isaacabraham setting the environment variable TEST_UPDATE_BSL to 1 and running the test should update your .bsl files.

By running build -TEST or is there a quicker way of doing it?

@smoothdeveloper
Copy link
Contributor

You can hijack the check of that variable:

fsharp/tests/FSharp.Test.Utilities/Compiler.fs

Line 1121 in 2c3bdcb

if updateBaseline () then

fsharp/tests/FSharp.Test.Utilities/SurfaceArea.fs

Line 74 in 2c3bdcb

match Environment.GetEnvironmentVariable("TEST_UPDATE_BSL") with

Or if you run the suite from command line, using set TEST_UPDATE_BSL=1 per the dev guidelines:

fsharp/DEVGUIDE.md

Line 192 in 2c3bdcb

set TEST_UPDATE_BSL=1

@isaacabraham
Copy link
Contributor Author

@isaacabraham looks like the only error apart from lack of release notes was this crash. If it passes locally it is most probably good.

Any idea how to make this green?

@majocha
Copy link
Contributor

majocha commented Dec 25, 2024

Any idea how to make this green?

Just rerun the CI and hopefully the tests will not crash the CLR this time. To rerun you can close and reopen the PR or just add another commit with release notes. Sorry for the tests being a bit unstable in CI right now. I hope this will help when merged: #18169.

@dotnet dotnet deleted a comment from github-actions bot Dec 26, 2024
@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Dec 30, 2024
T-Gro
T-Gro reviewed Dec 30, 2024
View reviewed changes
src/Compiler/Driver/CompilerDiagnostics.fs
@@ -678,8 +678,7 @@ type Exception with

let t1, t2, _cxs = NicePrint.minimalStringsOfTwoTypes denv ty1 ty2

os.Append(ConstraintSolverNullnessWarningEquivWithTypesE().Format t1 t2)
|> ignore
os.Append(ConstraintSolverNullnessWarningEquivWithTypesE().Format t1) |> ignore
Copy link
Member

Choose a reason for hiding this comment

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

The change is probably correct, but I would prefer to see at the throw site that indeed the "t1" type is always the non-nullable one and t2 is nullable.

This can be e.g. done by making the pattern match in SolveNullnessEquiv total.
Also, the t2 field is therefore not needed anymore if I got it correctly, the same for minimalStringsOfTwoTypes ?

(= new textual message now displays only the first type and assumes the other ALWAYS is a nullable version of the first)

Copy link
Contributor Author

@isaacabraham isaacabraham Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That assumption is correct, yes. I couldn't see where it wouldn't be the case - if the LHS is nullable and the right-hand side isn't, it's safe and the compiler will implicitly allow the change in type, right? i.e.

let x = ""
let y : string | null = x // this is fine, no warning is generated

@isaacabraham
Copy link
Contributor Author

Any idea how to make this green?

Just rerun the CI and hopefully the tests will not crash the CLR this time. To rerun you can close and reopen the PR or just add another commit with release notes. Sorry for the tests being a bit unstable in CI right now. I hope this will help when merged: #18169.

For the release notes, what can I do to fix that - which file needs updating?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@T-Gro T-Gro T-Gro left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
F# Compiler and Tooling
Status: New
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve Error Reporting: Make Nullness Warning 3261 less intimidating - Part 2
4 participants
@isaacabraham @majocha @smoothdeveloper @T-Gro