Skip to content
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

rust_verify_test: support expected message in FAILS comment #1200

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mmcloughlin
Copy link
Contributor

This PR extends the testing mechanism that checks for FAILS comments to additionally allow an expected error message.

A comment of the form // FAILS just checks for an error. The form // FAILS: <expect> also confirms that <expect> appears in the diagnostic text.

This PR demonstrates the feature by also updating some of the integer_ring tests.

@mmcloughlin
Copy link
Contributor Author

I'm not sure if this is a good idea, open to suggestions.

A simple alternative is something like a helper assert_one_fails_with_vir_error_msg. This inline comment approach could be nice for the multi-error case though.

If we are going to support this kind of extended // FAILS syntax, we might also want to think about enforcing a format for FAILS comments. At the moment there are some comments with notes attached to them, for example:

assert(!b); // FAILS (while converted to loop due to ensures)

assert(a % a == 0); // FAILS , `x` shouldn't be zero

Perhaps we support both an expectation and a note?

// FAIL
// FAIL: <expect_msg>
// FAIL (<note>)
// FAIL: <expect_msg> (<note>)

@mmcloughlin mmcloughlin marked this pull request as ready for review June 29, 2024 02:16
@utaal
Copy link
Collaborator

utaal commented Jul 1, 2024

Hi @mmcloughlin. Thank you for the PR.

I have a fairly strong preference for the alternative approach:

A simple alternative is something like a helper assert_one_fails_with_vir_error_msg.

The FAILS comment, in my mind, should just be a marker of the affected line, and I'd prefer we kept that as simple as possible. Multiple errors with FAILS are uncommon, because we generally will only reliably get one SMT failure (the earliest failing assertion), and so we don't generally write tests with multiple FAILS.

Support for info or warning would be useful, through the addition of other markers (there it would be more useful to have multiple -- and perhaps identify them by some "id" to match them with the expected message. In fact, we could also do this for "FAILS" in the uncommon case where there are multiple FAILS we want to handle, e.g.


use vstd::string::*;
fn get_char_fails() {
    let x = new_strlit("hello world");
    let val = x.get_char(0); // FAILS(0)
    assert(val === 'h'); // FAILS(1)
}

uses:

assert_fails_with_vir_msgs(err, [
    Level::Error, "invalid comparison operator `===`",
    Level::Error, "expected function, found macro `assert`",
])

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