Skip to content

Type checker: don't suppress errors while checking expressions#18311

Merged
T-Gro merged 14 commits into
dotnet:mainfrom
auduchinok:tcExpr-noErrorSuppress
Apr 9, 2025
Merged

Type checker: don't suppress errors while checking expressions#18311
T-Gro merged 14 commits into
dotnet:mainfrom
auduchinok:tcExpr-noErrorSuppress

Conversation

@auduchinok

Copy link
Copy Markdown
Member

Fixes #17787.

I guess many tests may need to be updated. 🙂

@auduchinok auduchinok requested a review from a team as a code owner February 12, 2025 17:41
@github-actions

github-actions Bot commented Feb 12, 2025

Copy link
Copy Markdown
Contributor

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.300.md

@psfinaki

Copy link
Copy Markdown
Contributor

Well - 11 tests, that's much less than I would have expected here :)

Comment thread tests/fsharp/typecheck/sigs/neg45.bsl Outdated
Comment thread docs/release-notes/.FSharp.Compiler.Service/9.0.300.md Outdated
@auduchinok auduchinok force-pushed the tcExpr-noErrorSuppress branch from e025765 to a8c081a Compare February 20, 2025 18:32
@auduchinok

auduchinok commented Mar 13, 2025

Copy link
Copy Markdown
Member Author

Could someone please help me with updating the baselines here? I've tried committing the files produced during the local tests run, but it doesn't seem to succeed on CI.

@Martin521

Martin521 commented Mar 13, 2025

Copy link
Copy Markdown
Contributor

This does not look like a baseline issue, all the CI runs today had these failures.
See also here.

EDIT: sorry, forget that, seems to be different

@auduchinok

Copy link
Copy Markdown
Member Author

Could someone please help me with updating the baselines here? I've tried committing the files produced during the local tests run, but it doesn't seem to succeed on CI.

@psfinaki

psfinaki commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

Hi @auduchinok sorry for the long delay here. I finally got to this - I pushed the changes and it's all green now.

You were actually close. The thing is - there are 2 kinds of baselines here, .bsl and .vsbsl - and this test required adding the latter. Don't ask me why it's like this... artifacts of earlier testing approaches, you know.

@auduchinok

Copy link
Copy Markdown
Member Author

@psfinaki Thank you very much!

@auduchinok

auduchinok commented Apr 8, 2025

Copy link
Copy Markdown
Member Author

The tests seem to fail because of #4516. I wish we could finally fix it and not create workarounds in the tooling and tests.

@auduchinok

Copy link
Copy Markdown
Member Author

This is ready.

There was some difference between the local run and the CI in how the overload resolution error is reported. This could be because of how FCS tests build project options, which may be somewhat different from the CI. I've removed the diagnostic messages from the new tests for now.

@T-Gro T-Gro enabled auto-merge (squash) April 9, 2025 13:00

@psfinaki psfinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job, thanks!

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.

Type checker errors should be reported when syntax errors are present

5 participants