Skip to content

checker: reject ordered comparisons of operands without a common order - #28854

Open
MARCROCK22 wants to merge 3 commits into
vlang:masterfrom
MARCROCK22:fix-ordered-comparison-operands
Open

MARCROCK22 wants to merge 3 commits into
vlang:masterfrom
MARCROCK22:fix-ordered-comparison-operands

Conversation

@MARCROCK22

Copy link
Copy Markdown

Fixes #28851

<, >, <= and >= had their operands checked only when both were arrays, fn values or structs. Anything else went to cgen as it was: 'A string' < 1, a thread handle, a map, a sum type or a struct compared with a number failed in the C compiler, and a bool, an enum, a channel or a fn value compared with a number compiled into a comparison that means nothing.

The ordered operators now get the compatibility check that == already has, with the messages V1 gives for the same code. For the example in the issue:

test.v:2:4: error: infix expr: cannot use `int literal` (right expression) as `string`
    1 | a := 'A string'
    2 | if a < 1 {
      |    ~~~~~

Numbers still compare with any number, a pointer with an integer, a voidptr with what C can order it with, and a struct with its own < only with its own type. Maps, interfaces and ?T < none, which V1 also let through to the C compiler, are rejected too.

Also in this PR:

  • unsafe { x == 0 } is accepted only when x is a struct reached through a pointer, such as a mut parameter; with a struct value it failed in the C compiler, and the suggestion to use unsafe is only given where it helps.
  • The conditions of match branches, as in match true { a < b {} }, now get the checks an if condition gets; none of them ran before.
  • Finding the operator of a comparison for a diagnostic no longer slices past the end of the V file that template code is attributed to.

Tests: vlib/v/compiler_tests/ordered_comparison_operands_test.v covers a 33×33 matrix of operand kinds for the four operators, V1's messages with their columns, the contexts a comparison can be in (filter callbacks, it, if, for, assert, closures, match branches, methods), a program with valid comparisons that has to compile and run (generics, sort, smartcasts, options, mut parameters, $for, operator overloads), templates, .sort() comparators and @[translated] files.

`<`, `>`, `<=` and `>=` had their operands checked only when both were
arrays, fn values or structs. Anything else went to the C backend as it
was: a thread handle, map, sum type or struct compared with a number
failed in the C compiler, and a bool, enum, channel or fn value compared
with a number compiled into a comparison that means nothing. They now get
the compatibility check of `==`, with the messages V1 gives for them:
numbers compare with any number, a pointer with an integer, a `voidptr`
with what C can order it with, and a struct with its own `<` only with
its own type.

V1 let three more kinds of operands reach the C compiler, and they are
rejected too: maps, interfaces, and an option against `none`.

`unsafe { x == 0 }` is accepted only when `x` is a struct reached through
a pointer, such as a `mut` parameter, and only there does the checker
suggest `unsafe`: a struct value compared with zero failed in the C
compiler.

The conditions of `match` branches, as in `match true { a < b {} }`, now
get the checks an `if` condition gets; none of them ran before.

Finding the operator of a comparison for a diagnostic no longer slices
past the end of the V file that template code is attributed to, whose
offsets can run beyond it.

@medvednikov medvednikov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at d8bd3cfeb6b24a17b3f9f4bf9feb97fcfd012557.

P2 — Check wrapped match-condition expressions, not only bare infix nodes. In vlib/v/types/checker_tail_stmt.v, the new check_match_stmt call to check_node(cond_id) is guarded by kind == .infix. A condition such as !(t > 1) has a prefix node at its root, and (t > 1) has a parenthesis node, so the comparison inside it misses this new checking path. The subsequent check_match_condition_type resolves the condition's type and accepts a bool against match true; it does not substitute for checking the nested comparison operands. Please traverse value-condition expressions through these wrappers while retaining the separate handling of type patterns and ranges. Extend the context regression with negated and parenthesized versions of its existing thread-handle-versus-integer match condition, and assert an error at the inner comparison.

P2 — Quote executable and file paths in the new test helpers. build_v3, check_file_errors, run_good, and the template test interpolate executable, input, and output paths directly into shell commands. A checkout such as /tmp/v review, or a temporary directory containing spaces, splits those paths into separate command words; the new tests then fail before exercising the checker. Quote each path with os.quoted_path, including the executable used by os.execute(out)/os.execute(good_bin). The quoted -path argument alone does not protect the other arguments.

The broad operand matrix, exact diagnostic assertions, and compile-and-run coverage are useful. I also checked the existing operator-overload precedence, wrapped-value checks, struct comparison path, sort-comparator handling, and the added source-slice bounds guard.

Validation: complete five-file diff, all of the new test file, and the surrounding checker/match code were inspected. These findings are from source review, not an executed V reproducer. No V compiler is available locally, so I did not rebuild or run the tests. CI status and results were not considered.

A match condition such as `!(t > 1)`, `(t > 1)` or `is_true(t > 1)` has a
prefix, parenthesis or call node at its root, so the comparison inside it was
still left unchecked. Every condition that is an expression now gets the
checks of an `if` condition; names, which can stand for types, type patterns,
ranges, enum shorthands and `none` keep the checks of their own.

The test helpers quote the paths they pass to the shell, so a checkout or a
temporary directory whose path has spaces works.

@medvednikov medvednikov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed at e11c223dd25747167ff6fa37a27afd83b23050d6, including the latest fix. CI status/results were not considered.

The previously reported negated/parenthesized match conditions now reach check_node, and the new call-wrapped negative case is useful. The executable, input, output, and compiler search paths in the test helpers are now quoted with os.quoted_path.

P2 — Do not exclude selectors whose receivers are expressions (vlib/v/types/checker_tail_stmt.v:1549–1554). match_condition_is_expression returns false for every .selector, not just qualified type names. This still skips the new expression-checking traversal for a condition such as predicate(t > 1).value: its root is a selector, but its receiver is a call containing the invalid comparison. A selected field having type bool does not establish that the receiver's arguments are valid.

Suggested regression, alongside the new call-wrapped case:

struct Predicate {
    value bool
}

fn predicate(value bool) Predicate {
    return Predicate{value: value}
}

fn check(t thread int) {
    match true {
        predicate(t > 1).value {}
        else {}
    }
}

fn main() {}

Please distinguish qualified type-pattern selectors from selectors with value-expression receivers, and ensure the latter's receiver subtree is checked. Assert an operand-mismatch diagnostic at t > 1, and retain a valid selector-condition case plus qualified type-pattern coverage to avoid breaking type matches. The same exclusion also deserves a case with a struct-initializer receiver.

Validation: inspected the latest two-file fix, the complete current five-file diff, and surrounding match-checking code. The selector concern above is a source-review finding; the proposed regression was not executed. No V compiler is installed in this environment, so I did not rebuild or run the tests.

A match condition such as `predicate(t > 1).value` has a selector at its
root, and every selector was left unchecked because `mod.Type` is one too.
Only a chain of names, which can be a qualified type, is left out now; a
selector on a call, a struct initializer, an index or any other expression
gets the checks of an `if` condition, so a comparison in its receiver is
reported.

The contexts test also checks that each error is a mismatch of the
operands, reported at the comparison.

@medvednikov medvednikov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed the latest fix at 377ac1fdf3eb8e24e77c2ebc8bf4066d12d1b557. CI status/results were not considered.

The selector P2 from my previous review is addressed. match_condition_is_expression now walks through selector receivers to distinguish a chain of names from a selector rooted in a value expression. Importantly, check_match_stmt still passes the original condition to check_node, so the complete expression is checked rather than just the receiver used for classification. Qualified names such as time.Time remain on the existing type-pattern path.

The added negative cases cover the reported predicate(t > 1).value example, a struct-initializer receiver, chained selectors, and an indexed receiver containing an invalid comparison. The context test now requires an operand-mismatch diagnostic at the comparison's column, rather than accepting any error on the same line. The compile-and-run test also adds valid selector conditions and a qualified time.Time sum-type pattern.

No new actionable findings in this follow-up review. The previously reported wrapped-condition and shell-path-quoting fixes remain in place.

Validation: inspected the latest two-file commit, the updated context and positive regression tests, and the surrounding match-checking code. This is a source-review conclusion, not a test-pass claim: no V compiler is available locally, so I did not rebuild or execute the tests.

This branch has not been deployed

No deployments
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.

invalid comparison string < int is allowed, fails to compile

2 participants