Skip to content

fix(compiler): diagnose optional binary operands - #9008

Open
chess10kp wants to merge 4 commits into
jaseci-labs:mainfrom
chess10kp:fix/8547-optional-binary-diagnostics
Open

fix(compiler): diagnose optional binary operands#9008
chess10kp wants to merge 4 commits into
jaseci-labs:mainfrom
chess10kp:fix/8547-optional-binary-diagnostics

Conversation

@chess10kp

Copy link
Copy Markdown
Collaborator

Summary

  • diagnose un-narrowed optional operands before binary numeric promotion and magic-method resolution
  • preserve direct equality and union-annotation handling
  • cover forward and reflected arithmetic with the existing native optional diagnostics fixture

Reproduction

Before this change, jac check --native-coverage accepted return x + 1 for x: int | None without an optional-operand diagnostic. The attribute-access twin still reported its existing optional error.

Verification

  • jac check --native-coverage /tmp/jac-8547-repro.jac reports E1121 for x + 1
  • native fixture compilation reports E1121 for both x + 1 and 1 + x
  • the one targeted test-file run was blocked before tests by the installed 0.36.1 launcher rejecting pre-existing ctypes type signatures in the current test file

Closes #8547

@christianwilkins christianwilkins left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed head bf3abd00ce57b7ca21ca9b705d0643420b67bfd2, including the arithmetic diagnostic, the follow-up loop guards, the native fixtures, the optional-diagnostic helper, and the binary-operation dispatch around the new guard.

P2: runtime bitwise OR bypasses the new optional-operand check. With x: int | None, both x | 1 and 1 | x still produce no optional diagnostic. The union-annotation path already returns above this guard; exempting every BW_OR also exempts runtime operands. Please cover both directions and narrow the exemption to actual annotations.

Local validation: a direct JacProgram.compile probe with CompileOptions(force_codespace="native", force_target_program=True) produced E1121 for forward addition, reflected addition, and bitwise AND, but neither OR expression. Its assertion expecting all five diagnostics failed (3 reported); guarded addition and optional equality produced no errors. An initial test-helper import hit the existing ctypes-signature checking obstacle described in the PR. A separate temporary-change control did not finish compilation and is not counted as validation; the source was restored and the worktree is clean.

Current-head CI is green (25 successful checks, 3 skipped), but GitHub reports merge conflicts. Changes requested: fix and test runtime OR, resolve the conflicts while preserving the follow-up narrowing guards, and rerun validation/CI on the resulting head. Maintainer review/merge follows after those author actions.

}
(magic, rmagic) = BINARY_OPERATOR_MAP[expr.op.name];

if expr.op.name not in (Tok.EE, Tok.NE, Tok.BW_OR) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P2] Do not exempt runtime bitwise OR from the optional-operand diagnostic. The annotation-only OR case already returns at lines 354–363. Here, x | 1 and 1 | x with x: int | None skip the guard and still emit no E1121. A forced-native compile of those cases alongside x + 1, 1 + x, and x & 1 reports only the latter three errors. Limit the exemption to actual union annotations and add both runtime OR directions to the regression fixture.

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.

[Bug] Binary operators on Optional values bypass None-narrowing (x + 1 on int | None passes)

2 participants