Improve unclear error and warning messages#8460
Open
JonoPrest wants to merge 30 commits into
Open
Conversation
Saying "The function has type" right after "it's not a function" was contradictory. Use "It has type" instead.
The old message was grammatically broken ("expect return type to be syntax
wise `_ option` for safety") and rendered `%@return` literally. State the
requirement plainly and show the option syntax.
Fix the "%@this" rendering and the broken grammar ("expect its pattern variable to be simple form"). State what is required and why.
Capitalize and use a full sentence instead of "expect string literal " (trailing space). Kept generic since it is shared by @as payloads and @@directive.
Replace "expect int, string literal or json literal {json|text here|json}"
with a plain sentence.
Verified against the raise sites: - @this (ast_uncurry_gen.ml): the self parameter must satisfy is_single_variable_pattern_conservative, i.e. a plain variable or `_`. A constant pattern also fails and `_` is allowed, so "not a destructured pattern" was imprecise. State the requirement positively. - @return (ast_external_process.ml): `nullable` and `null_undefined_to_opt` trigger this too, not just `*_to_opt` directives, so describe the option requirement generically rather than naming the directive shape.
Replace the OCaml-style `let rec' quoting with `let rec`.
Warnings 52 and 57 pointed users to "manual section 8.5" of the OCaml manual, which is irrelevant to ReScript. Also add a space after the comma when listing ambiguous or-pattern variables.
List the supported directives instead of the terse "Not supported return directive".
Duplicated_bs_as also fires for record fields (e.g. inline records) and polymorphic-variant tags, not just variant cases, so the message should not say "a variant case".
"is not allowed in programs" was vague; type variable names cannot start with an underscore.
"expects 1 argument(s), but is here applied to 2 argument(s)" -> proper singular/plural and "is given".
- first-class module instead of "packed module" - explain that a cross-module GADT constructor needs its module prefix - explain the existential-in-pattern error and point to switch
Quote the name like the other polished messages (and the same message's zero-argument branch already does).
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
JonoPrest
force-pushed
the
jono/improve-error-messages
branch
from
53f37dd to
df15a5d
Compare
Errors are sentence-case; "unsupported predicates" and the consecutive- statements syntax error started lowercase.
JonoPrest
force-pushed
the
jono/improve-error-messages
branch
from
df15a5d to
4b6a19c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8460 +/- ##
=======================================
Coverage 60.93% 60.94%
=======================================
Files 374 374
Lines 54039 54050 +11
=======================================
+ Hits 32930 32942 +12
+ Misses 21109 21108 -1
🚀 New features to boost your workflow:
|
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
- Add a fixture for a non-generic type applied to arguments (covers the zero-arity branch of the type-arity-mismatch message, and the plural case). - Add an @ignore/@unwrap conflict fixture (covers those attr_name arms). - Drop the unreachable `Nothing` arm from attr_name by typing the collected list to the four real tags and coercing the single-tag result.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Improves error and warning messages that were unclear, terse, or used OCaml
terminology. Message text only, no behaviour changes; every change is covered
by an updated super_errors fixture.
Details
Follow-up to the super_errors fixture coverage work. After reviewing the
fixtures added in that work (including the ones flagged by @cknitt on #8432),
the changes group into:
@return(*_to_opt),the
@int/@aspayload, and conflicting attributes.@correctly in FFI attribute messages, which printed%@literally.switch(not "match expression"),pattern (not "matching"),
let rec(notlet rec'), first-class module(not "packed module").
in the type-arity error, and dropping the OCaml-manual references from two
warnings.
(e.g.
@string, @int) instead of listing every possible one.@inlinewarning, which wasleaking an internal
inline1.A few advanced type-system messages (variance, polymorphic-variant
present/conjunction) are left as-is; rewording them needs more care.