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

test: Show error when using a binary operator without two operands #6193

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sargas
Copy link
Contributor

@sargas sargas commented Apr 5, 2024

When preforming a binary operation with the test command, GNU coreutils gives an error if only one operands exist:

$ /bin/test -o blah
/bin/test: ‘-o’: unary operator expected
$ /bin/test -a blah
/bin/test: ‘-a’: unary operator expected

$ /bin/test blah -a
/bin/test: missing argument after ‘-a’
$ /bin/test blah -o
/bin/test: missing argument after ‘-o’

This PR adds the error messages for the first set, where the first operand is missing. The second operand missing does not appear as trivial, since Symbol::new causes the operator to be treated as a literal as part of the parsing.

With this PR, tests/test/test-diag.pl should pass.

This PR also fixes an integration test that says that -a ! should return an exit code of 1. GNU test gives an error with a status code of 2.

@sargas sargas changed the title test: Show error when using a binary operator withut two operands test: Show error when using a binary operator without two operands Apr 5, 2024
@sargas sargas marked this pull request as draft April 5, 2024 05:47
@sargas sargas marked this pull request as ready for review April 5, 2024 05:50
Copy link

github-actions bot commented Apr 5, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/test/test-diag is no longer failing!

@@ -67,7 +67,7 @@ fn test_double_not_is_false() {

#[test]
fn test_and_not_is_false() {
new_ucmd!().args(&["-a", "!"]).run().code_is(1);
new_ucmd!().args(&["-a", "!"]).run().code_is(2);
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

could you please add a check of the string ? thanks

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

ping ? :)

@tertsdiepraam
Copy link
Member

@sargas

The second operand missing does not appear as trivial, since Symbol::new causes the operator to be treated as a literal as part of the parsing.

This should still be done right? Should we open an issue for it in meantime?

@sargas
Copy link
Contributor Author

sargas commented Apr 5, 2024

@sargas

The second operand missing does not appear as trivial, since Symbol::new causes the operator to be treated as a literal as part of the parsing.

This should still be done right? Should we open an issue for it in meantime?

Agreed, especially since the GNU tests didn't check for this. I can have a go at this tonight

@sylvestre
Copy link
Sponsor Contributor

@sargas ping ? :)

Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Thank you! Yes, this PR fixes the GNU test, but just shifts the problem in an unfixable way, because after parse() has finished, important information is lost.

Can the error be detected during parse()?

Comment on lines +166 to +168
if stack.is_empty() {
return Err(ParseError::UnaryOperatorExpected(op.quote().to_string()));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works on the top-level, yes, but does not work if there are preceding expressions, e.g. test foo -a ( -o bar ).

In fact, this cannot work, since the information is lost after parse() is finished, and your code raises the error only later in eval. See #6203 for more examples.

Therefore, the error handling must happen during parse(), and doing it in eval() would only make it even harder to get it right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woah, thank you for the detailed explanation in that issue. I'll take a look at how the parsing is done to see what's simple to change and watch the other issue for updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to leave an update on this: I looked into using nom and pest for an easier to update grammar, and starting implementing the grammar in pest, but this was an awkward approach. test operates on a vector, while parsers usually operate on strings or something tokenized. Still trying things out, but wanted to relay I'm still interested in this

@sargas
Copy link
Contributor Author

sargas commented Apr 8, 2024

@sargas ping ? :)

Sorry, busier than expected, still interested in this PR. Whether <blah> -a shows an error is different for an arbitrary string and for !, so I wanted to create some more test cases

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/test/test-diag is no longer failing!

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.

None yet

4 participants