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

Bugfix :: Nullness in signature file is not considered by implementation and vice versa #18186

Merged
merged 63 commits into from
Feb 6, 2025
Merged

Bugfix :: Nullness in signature file is not considered by implementation and vice versa #18186

merged 63 commits into from
Feb 6, 2025

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented Dec 30, 2024 •
edited
Loading

Fixes #18058
Fixes #18184
Fixes #18185

@T-Gro T-Gro linked an issue Dec 30, 2024 that may be closed by this pull request
Nullness issue - nullness in signature file is not considered by implementation and vice versa #18058
Closed
7 tasks
T-Gro added 26 commits December 30, 2024 15:27
@T-Gro
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
c0d4991 
…-file-is-not-considered-by-implementation-and-vice-versa
@T-Gro
fantomasd
307eaef
@T-Gro
Merge branch '18058-nullness-issue---nullness-in-signature-file-is-no…
3e1b650 
…t-considered-by-implementation-and-vice-versa' of https://github.com/dotnet/fsharp into 18058-nullness-issue---nullness-in-signature-file-is-not-considered-by-implementation-and-vice-versa
@T-Gro
Apply changes to codebase
5bd7b8b
@T-Gro
adjust code to meet signature conformance
09d0232
@T-Gro
see errors with ambivalence turned on
05db836
@T-Gro
allow (WithoutNull === Ambivalent) in signature conformance
c758e17
@T-Gro
conform impl to signature
dd55064
@T-Gro
fix
6123665
@T-Gro
a few more
f87f1fb
@T-Gro
format
79e0e52
@T-Gro
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
b58290b 
…-file-is-not-considered-by-implementation-and-vice-versa
@T-Gro
few more
582f9af
@T-Gro
Can I get all errors at once if I replace them with warnings?
21f9e20
@T-Gro
some more
c1ba534
@T-Gro
parsing
3f2ae24
@T-Gro
one more
9c300b2
@T-Gro
try get more errors at once
703e821
@T-Gro
That one should stay strict - this is checked per-phase (OK), not per…
a0ef43e 
… file
@T-Gro
Can I process more diags now?
d30504c
@T-Gro
Revisit this - this produced a "mismatch on accessibility" first
c67b453
@T-Gro
another
8449fc4
@T-Gro
cannot see info warning in output
b1635c0
@T-Gro
i really want to see all warnings
d8fa0e6
@T-Gro
prettynaming conformant now
dc3e815
@T-Gro
ilreflect conformant
9cc5227
@T-Gro T-Gro marked this pull request as ready for review February 3, 2025 15:49
@T-Gro T-Gro requested a review from a team as a code owner February 3, 2025 15:49
@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Feb 4, 2025
@T-Gro
Copy link
Member Author

T-Gro commented Feb 4, 2025

(Compiler release notes are there. I am silencing the demand for Fsharp.Core release notes because the changes are immaterial - xmldoc changes, a missing annotation and removal of overrides from .fsi files)

@dotnet dotnet deleted a comment from github-actions bot Feb 4, 2025
@KevinRansom
Merge branch 'main' into 18058-nullness-issue---nullness-in-signature…
bba18bc 
…-file-is-not-considered-by-implementation-and-vice-versa
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 4, 2025

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

KevinRansom
KevinRansom requested changes Feb 4, 2025
View reviewed changes
Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

@tomas, this looks good, but I have a small question on
onlyDiffersInNullness generating a warning, which I hope you can clear up for me. Other than it looks great

.../FSharp.Compiler.ComponentTests/EmittedIL/Nullness/HasSignatureWithMissingOverride.fs.il.bsl Outdated Show resolved Hide resolved
src/Compiler/Checking/MethodCalls.fs Show resolved Hide resolved
src/Compiler/Checking/NicePrint.fs Show resolved Hide resolved
src/Compiler/Checking/NicePrint.fs Outdated Show resolved Hide resolved
src/Compiler/Checking/SignatureConformance.fs Show resolved Hide resolved
@tomas
Copy link

tomas commented Feb 4, 2025

@KevinRansom please do not mention me again in this repo.

@KevinRansom
Copy link
Member

@KevinRansom please do not mention me again in this repo.

Well, there have been quite a few Tomas' that have worked on F# over the years. I will do my best not to mention you, but you do have the coolest alias.

0101
0101 approved these changes Feb 5, 2025
View reviewed changes
tests/FSharp.Test.Utilities/Compiler.fs Outdated Show resolved Hide resolved
src/Compiler/Driver/ParseAndCheckInputs.fs Show resolved Hide resolved
@T-Gro T-Gro enabled auto-merge (squash) February 6, 2025 10:48
@T-Gro T-Gro merged commit 841ba8e into main Feb 6, 2025
33 checks passed
@T-Gro T-Gro deleted the 18058-nullness-issue---nullness-in-signature-file-is-not-considered-by-implementation-and-vice-versa branch February 7, 2025 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0101 0101 0101 approved these changes

@KevinRansom KevinRansom KevinRansom approved these changes

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
Archived in project
Milestone
No milestone
4 participants
@T-Gro @tomas @KevinRansom @0101