This always succeeds on unix platforms, and fails on other platforms if the string can't be coerced to UTF-8.

Is it really ok to fail here?

• If the string doesn't start with a '/", we probably still want to move forward to extended_parse?
• Is there something we can do to still try to figure out if the first char is a \'? (maybe we should use os_str_as_bytes_lossy?!)

(I was looking at this wondering if we really need to result a Result<T, ...> instead of just T, as that'd avoid a lot of the other changes... But I guess we'd still need to fail if the first character is ' and the second is U+FFFD REPLACEMENT CHARACTER.)

Maybe this could help but it's not stable... https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.slice_encoded_bytes

• If the string doesn't start with a '/", we probably still want to move forward to extended_parse?

Well, there are no numbers that contain a unicode replacement character, so it would fail anyway. 😛 I think it's fine to fail early here, the string being invalid unicode is likely to be about as useful error context as the string not being a valid number.

• Is there something we can do to still try to figure out if the first char is a \'? (maybe we should use os_str_as_bytes_lossy?!)

The only case we would get such a failure is on Windows with a string that is not valid unicode. Unlike unix platforms, this almost never happens, since all Windows strings are UTF-16(ish) -- namely, Windows doesn't allow arbitrary byte sequences (or byte pairs) as input, it has to look like valid UTF-16(ish). The exception (the "ish") is that Windows does allow invalid surrogate pairs, which can't be converted to bytes in any meaningful way, which is why conversion to bytes has to be fallible. This means that any argument here containing an invalid codepoint also necessarily contains data that doesn't represent any particular byte, which, aside from being difficult to do in practice, I can't think of any reason you would want that data to then be parsed as a literal -- it seems much more likely that something has gone horribly wrong.

But I guess we'd still need to fail if the first character is ' and the second is U+FFFD REPLACEMENT CHARACTER.

Problem with that strategy is we'd need some way to distinguish the replacement character from lossy conversion and the replacement character as input.

• If the string doesn't start with a '/", we probably still want to move forward to extended_parse?

Well, there are no numbers that contain a unicode replacement character, so it would fail anyway. 😛 I think it's fine to fail early here, the string being invalid unicode is likely to be about as useful error context as the string not being a valid number.

I should have made my thought clearer... In general, partial matches still return whatever can be parsed (with an error). And you do the same here with ' prefixes (see my new comment below though, you do change the behaviour a bit by manually printing the error instead of returning a PartialMatch)

So I think something like 123abc[badunicode] will return PartialMatch(123, "abc[U+FFFD]") with your change, I think I'm ok with that? But you do reject \'abc[badunicode], wouldn't be better if we could still return the ASCII value of a here? (ideally a PartialMatch(97, "abc[U+FFFD]").

• Is there something we can do to still try to figure out if the first char is a \'? (maybe we should use os_str_as_bytes_lossy?!)

The only case we would get such a failure is on Windows with a string that is not valid unicode. Unlike unix platforms, this almost never happens, since all Windows strings are UTF-16(ish) -- namely, Windows doesn't allow arbitrary byte sequences (or byte pairs) as input, it has to look like valid UTF-16(ish). The exception (the "ish") is that Windows does allow invalid surrogate pairs, which can't be converted to bytes in any meaningful way, which is why conversion to bytes has to be fallible. This means that any argument here containing an invalid codepoint also necessarily contains data that doesn't represent any particular byte, which, aside from being difficult to do in practice, I can't think of any reason you would want that data to then be parsed as a literal -- it seems much more likely that something has gone horribly wrong.

Goodness, thanks for the explanation...

If this is exceedingly rare, I do wonder if it's not easier to just print a warning, and return a best guess value (even if that ends up being U+FFFD due to the lossy replacement...).

Okay... back from running after my train. I think my thoughts are getting a bit clearer. I'll continue my point from below here to keep things in one place.

I'm not terribly happy that get_num returns a Result, it's really weird that it propagates the errors from literal strings \' (in a very uncommon non-Unix corner case BTW), but processes the errors from extended_parse to the best possible number.

I'm also not too happy to move code out of extended_parse (I've been spending a lot of time trying to move all the parsing code in one place...). But I also understand we may not want to pass OsStr to extended_parse... So maybe it's ok to do this processing here (I'd just add a comment in extended_parse), since argument.rs is the only user (AFAICT).

So I think I'd do something like this:

• fn get_num<T>(os: &OsStr) -> T
• let s = os.to_string_lossy();
• If s doesn't start with \', call extended_parse. If not, call another function parse_literal(?) with the original OsStr that also returns a Result<T, ExtendedParserError<'_, T>>
  • In parse_literal, you can call os_str_as_bytes and just return a NotNumeric if that fails (or feel free to create a new error) -- or maybe you can just use to_string_lossy and accept that we'd sometimes return 0xfffd, all of these options sound ok to me.
  • In parse_literal, you can return a PartialMatch instead of copy-pasting the warning.
• Then you can call extract_value on the return value from above (either extended_parse or the parse_literal)

That simplifies the code a bit, avoids a bunch of unwrap, removes the warning message duplication.

WDYT? I obviously didn't try, so maybe something doesn't work, or things can be done in yet another different way ,-)

I just realized you change the behaviour here, this used to return a PartialMatch. I'm not 100% sure if this is critical.

(this makes me wonder if it's a good idea to move this code out of num_parser.rs:parse)

[-- sorry I need to run, I'll try to think more about this later]