Skip to content

Don't filter out module-related type errors #1090

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

Merged
merged 2 commits into from
May 9, 2025

Conversation

mediremi
Copy link
Contributor

@mediremi mediremi commented May 9, 2025

The Not_included, With_mismatch and With_makes_applicative_functor_ill_typed errors from typemod.ml use Includemod.report_error to report errors, which includes filepaths in its output.

Like in #1086, by checking for the presence of these errors we're ensuring that the user will not incorrectly see a 'something has gone wrong with incremental typechecking' warning notification.

Comment on lines +686 to +691
d.message.startsWith("Multiple definition of the") ||
// The signature mismatch, with mismatch and ill typed applicative functor
// type errors all include the filepath with LOC
d.message.startsWith("Signature mismatch") ||
d.message.startsWith("In this `with' constraint") ||
d.message.startsWith("This `with' constraint on")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of manually baking in these conditions, we could instead check for stderr.trimStart().startsWith("We've found a bug for you") 🤔

@zth zth self-requested a review May 9, 2025 14:17
Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

This is great stuff!

We've talked about replacing this string processing approach with something like a proper JSON format instead. Maybe something worth looking into soon.

@zth zth merged commit 261335b into rescript-lang:master May 9, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants