Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f4f487f3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
compiler/ml/translmod.ml
Outdated
| | Tstr_attribute x -> | ||
| Builtin_attributes.warning_attribute ~ppwarning:false | ||
| ~report_attribute_errors:false x; |
There was a problem hiding this comment.
Preserve source-order scope for
@@warning directives
Calling Builtin_attributes.warning_attribute here mutates the global warning state, but transl_structure translates rem before translating many current items (for example Tstr_value and Tstr_module), so a @@warning placed mid-structure can incorrectly apply to declarations that appear earlier in source order. In practice this can wrongly suppress or promote warnings (including @@warnerror) on the wrong bindings, which changes build outcomes. The warning setting should be applied with proper scoping/restoration that matches source order instead of mutating global state in this branch.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 165476741a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| let warn_on_polymorphic_comparison loc = function | ||
| | Pobjcomp _ | Pobjorder | Pobjmin | Pobjmax -> | ||
| Location.prerr_warning loc Warnings.Bs_polymorphic_comparison |
There was a problem hiding this comment.
Do not warn on null/undefined equality primitives
Emitting Bs_polymorphic_comparison for every Pobjcomp here regresses the previous behavior for null/undefined checks: primitives like %equal_null / %equal_undefined / %equal_nullable (and ==/!= cases with a null/undefined constant) used to bypass warning 102 in lam_compile_primitive because they compile to dedicated null checks, but this early unconditional warning now fires before arguments are inspected. In projects using -warn-error +102, valid null/undefined comparisons can now fail compilation.
Useful? React with 👍 / 👎.
Fixes #8160.
Symptom
The reported repro was:
With warning
102enabled, this still produced:Root Cause
Warning
102(Bs_polymorphic_comparison) was emitted too late, incompiler/core/lam_compile_primitive.ml.By the time that code ran, local warning scopes created by source attributes such as
@warning("-102")had already been restored. As a result, file-wide warning settings still worked, but expression-level suppression did not affect warning102.Fix
The warning emission was moved into typedtree-to-lambda translation in
compiler/ml/translcore.ml, where it still runs under the relevant source-level warning scopes.Concretely:
102is emitted when the polymorphic comparison primitive is selected during translationcompiler/core/lam_compile_primitive.mlThis keeps warning
102enabled by default, while making local suppression behave correctly.Tests
Regression tests were added under
tests/build_tests/super_errors/:warning102_expr_attribute.reswarning102_structure_attribute.reswarning102_value_attribute.resThese cover:
@warning("-102")@@warning("-102")let v = @warning("-102") compareAll corresponding snapshots are empty, because successful suppression means there is no warning output.
Result
After the fix:
@warning("-102")works for local polymorphic comparison expressions@@warning("-102")still works102still appears normally when no suppression is present