Improve error messages for invalid binary strings by Sheape · Pull Request #16688 · nushell/nushell

Sheape · 2025-09-15T05:09:13Z

Fixes #9566. Based on the decision from this PR #16678, instead of dealing with out-of-range octal strings, throwing a specific error is better.

Let me know if these error messages are too detailed and prefer just having a single "out-of-range error".

Release notes summary - What our users need to know

Improved error messages for binary literals

Previously, a generic shell error is thrown for invalid binary strings. Now, an improve error message is thrown for invalid binary, hexadecimal, and octal strings.

> 0b[121]
# => Error: nu::parser::invalid_binary_string
# => 
# => × Invalid binary string.
# =>   ╭─[entry #1:1:1]
# => 1 │ 0b[121]
# =>   · ───┬───
# =>   ·    ╰── invalid binary string
# =>   ╰────
# =>  help: binary strings may contain only 0 or 1.

Tasks after submitting

N/A

Sheape · 2025-09-15T09:42:10Z

Sorry if I had to forced-push 😓. I had to resolve merge conflicts from Rust 1.88.0 PR.

sholderbach

Thanks for tackling this, looks good to me for the most part.

Welcome a second set of eyes regarding the integration into the chain of which erorrs are raised or fall back into a different parse.

sholderbach · 2025-09-15T21:27:13Z

crates/nu-parser/src/parser.rs

+    } else if !matches!(
+        working_set.parse_errors.last(),
+        Some(ParseError::Expected(_, _))
+    ) {


By looking at just the diff I can't immediately tell why this (negative) match is OK to pass with error suppresion.

Can you explain that briefly? Maybe it is worth adding a comment or a different match here.

This is similar to how parse_number matches (except without error supression). Before this change, the parser actually does not stop parsing and treats invalid binary strings as GlobPattern which is why I have to catch if the error is unexpected if that makes sense.

nushell/crates/nu-parser/src/parser.rs

Lines 1796 to 1808 in 529f6ec

pub fn parse_number(working_set: &mut StateWorkingSet, span: Span) -> Expression {

let starting_error_count = working_set.parse_errors.len();

let result = parse_int(working_set, span);

if starting_error_count == working_set.parse_errors.len() {

return result;

} else if !matches!(

working_set.parse_errors.last(),

Some(ParseError::Expected(_, _))

) {

} else {

working_set.parse_errors.truncate(starting_error_count);

}

The section you quoted is a horrible double negation with that empty else if ! branch.

I was confused for a second why the truncation in your code was fine but it makes sense in the is_math_expression_like hack where we don't want to leave errors.

If I understand the codepaths correctly the !matches on Expected filters down to just your newly created variant for InvalidBinaryString? Then I think it would make sense to use that and maybe add a comment why we choose to deviate here from the rest of the is_math_expression_like chain.

Whoops my bad, I genuinely thought that empty else if ! branch had a purpose (which was to not clear unexpected parsing integer error and fall through float parsing).

Yeah only that InvalidBinaryString variant should hit that codepath, or alternatively I can modify it to just strictly check if it matches InvalidBinaryString.

I'll add comments for this so people wouldn't get confused.

sholderbach · 2025-09-15T21:29:15Z

crates/nu-parser/src/parser.rs

+                2 => "binary strings may contain only 0 or 1.",
+                8 => "octal strings must have a length that is a multiple of three and contain values between 0o000 and 0o377.",
+                16 => "hexadecimal strings may contain only the characters 0–9 and A–F.",
+                _ => "", // radix other than 2, 8, or 16 is an invalid radix


Thanks for elaborating in the messages.

Defensively you can throw the text of the comment with something like INTERNAL ERROR: into the fall through arm. If there is ever something going wrong being loud about it can be helpful even if it is vanishingly likely to break.

crates/nu-parser/tests/test_parser.rs

sholderbach · 2025-09-15T21:34:09Z

crates/nu-protocol/src/errors/parse_error.rs

+    #[error("Invalid binary string.")]
+    #[diagnostic(code(nu::parser::invalid_binary_string), help("{1}"))]
+    InvalidBinaryString(#[label("invalid binary string")] Span, String),


It's a bit of a stylistic question if we need a completely separate variant for that or if we can cover it with our existing variants and some additional text (cc @cptpiepmatz )

This `else if` branch being empty makes no sense if there is no comment. Can be collapsed by removing the negation on the `matches!` Stumbled over this while reading nushell#16688 (comment)

sholderbach

After you can take a stab at the comment, clear to land from my side

github-actions bot added the A:parser Issues related to parsing label Sep 15, 2025

Sheape added 3 commits September 15, 2025 17:24

fix(parser): improved error message for binary strings

10ba9c6

fix(parser): truncate unexpected errors for binary strings

0473d09

test(parser): error test cases must expect invalid binary string

a06b11e

Sheape force-pushed the improve-error-msg-binary-str branch from 63f6b30 to a06b11e Compare September 15, 2025 09:39

sholderbach reviewed Sep 15, 2025

View reviewed changes

sholderbach mentioned this pull request Sep 17, 2025

Fix confusing double negation with empty else if #16708

Open

1 task

sholderbach approved these changes Sep 17, 2025

View reviewed changes

sholderbach mentioned this pull request Sep 17, 2025

0o[777] is not working #9566

Closed

Sheape added 2 commits September 18, 2025 03:58

chore(parser): added internal error message for decoding binary str

e907efd

chore: fix typo for comments on parsing binary strings

a4dbe6c

sholderbach merged commit 4062cbd into nushell:main Sep 17, 2025
16 checks passed

github-actions bot added this to the v0.108.0 milestone Sep 17, 2025

sholderbach added notes:ready The "Release notes summary" section of this PR is ready to be included in our release notes. notes:other Include the release notes summary in the "Other changes" section labels Sep 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error messages for invalid binary strings#16688

Improve error messages for invalid binary strings#16688
sholderbach merged 5 commits intonushell:mainfrom
Sheape:improve-error-msg-binary-str

Sheape commented Sep 15, 2025 •

edited by sholderbach

Loading

Uh oh!

Sheape commented Sep 15, 2025

Uh oh!

sholderbach left a comment

Uh oh!

sholderbach Sep 15, 2025

Uh oh!

Sheape Sep 15, 2025

Uh oh!

sholderbach Sep 17, 2025

Uh oh!

Sheape Sep 17, 2025

Uh oh!

sholderbach Sep 15, 2025

Uh oh!

Uh oh!

sholderbach Sep 15, 2025

Uh oh!

sholderbach left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	pub fn parse_number(working_set: &mut StateWorkingSet, span: Span) -> Expression {
	let starting_error_count = working_set.parse_errors.len();

	let result = parse_int(working_set, span);
	if starting_error_count == working_set.parse_errors.len() {
	return result;
	} else if !matches!(
	working_set.parse_errors.last(),
	Some(ParseError::Expected(_, _))
	) {
	} else {
	working_set.parse_errors.truncate(starting_error_count);
	}

Conversation

Sheape commented Sep 15, 2025 • edited by sholderbach Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes summary - What our users need to know

Improved error messages for binary literals

Tasks after submitting

Uh oh!

Sheape commented Sep 15, 2025

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

sholderbach Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Sheape Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

sholderbach Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Sheape Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

sholderbach Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sholderbach Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sheape commented Sep 15, 2025 •

edited by sholderbach

Loading