Fixes missing overflow lint for i64

[hirschenberger]

Fixes missing overflow lint for i64 #14269

The type_overflow lint, doesn't catch the overflow for i64 because the overflow happens earlier in the parse phase when the u64 as biggest possible int gets casted to i64 , without checking the for
overflows.
We can't lint in the parse phase, so we emit a compiler error, as we do for overflowing u64

Perhaps a consistent behaviour would be to emit a parse error for all overflowing integer types.

See #14269

[huonw]

Could you improve the commit message and PR description to actually describe what is being fixed, so that it stands somewhat independently of the issue?

[huonw]

Also, hm, shouldn't this be part of the type-overflow lint, not a syntax error?

[lilyball]

@huonw is correct, this is not an appropriate syntax error. type_overflow needs to be the one to emit this. Since as you say type_overflow can't even tell that the problem occurred, the correct solution is to figure out how the AST needs to change to fix this.

Personally, I think that integral literals in the AST simply need to support unsized values (or at least, absurdly-sized values). This will be necessary if we ever want to add support for user-defined types to be initialized from literals (e.g. BigInt).

A smaller change might be to add a bool flag to LitInt that indicates whether it overflowed during parsing. That could then be checked by the type_overflow lint and ignored everywhere else. Granted, that's still not a trivial change, because every single instance of LitInt in the source would need to be updated for the extra field, but at least it doesn't change the semantics of parsing and likely has a negligible performance impact (I can't imagine signed numeric literals are used frequently enough for it to matter).

[lilyball]

Alternatively, LitInt and LitUint could be combined into a single variant, with the value always stored as a u64, and a boolean flag added to indicate whether the literal value in the source was negative. Again, not a small change, but it does seem a bit odd to me to have two separate literal variants for integers. Actually it's 3, because there's also LitIntUnsuffixed. Seems to me all 3 should be a single literal type, with a type parameter that covers both signed and unsigned integral types, as well as the idea of unsuffixed (which would likely make it Option<IntTy> rather than being a separate variant).

[hirschenberger]

Your suggestions seem plausible for a clean implementation of a c-like behaviour.
But what about going further and break c-compatibility by forbiding the surprising behaviour of wrap-around integers literals and force the user to explicitly express the rarely used use pattern of wrap-around integer literals?

[lilyball]

@hirschenberger That doesn't work (during parsing), if for no other reason than that it cannot possibly work for unsuffixed literals. If the parser forbids wraparound of suffixed literals, people will expect it to forbid wraparound in general, and will get surprising behavior when the inferred type of an unsuffixed literal causes wraparound. This is why it's far more appropriate to put in a lint, which can actually warn about unsuffixed literals too.

[hirschenberger]

Yes, that's correct, i'll try to implement this, which of your suggested solutions do you prefer? I think combining all LitXXX types to one seems to be a sensible solution.

[lilyball]

I think combining all of LitXXXX types into one is ultimately the best solution, but I also don't think it's an easy change to make. And I don't know if there are any compelling reasons why we need the current split. This is the sort of thing that should be talked about with people who have much more experience with libsyntax than I do.

[hirschenberger]

Who would be appropriate? Can you CC someone or does it even need to be discussed in an RFC?

[lilyball]

@hirschenberger libsyntax changes generally don't need to be discussed in an RFC (assuming it's not changing the syntax or semantics of the language). Offhand I don't know who has the most experience with this sort of thing. Perhaps @cmr or @alexcrichton might have an opinion.

[alexcrichton]

No, small changes to the structure of libsyntax don't need an RFC. If you're feeling good you can mark them with breaking-change, but even that's not all too necessary.

I agree with others on this PR, many of our other messages about literals being out of range and such are all lints instead of compile-time errors, so it seems reasonable to keep up the trend.

[hirschenberger]

I merged the LitXX types as @kballard suggested and got it running, now overflowing i64 get a type_overflow lint, instead of a parse error.

Overflowing u64 still get parse errors. To avoid this, we could also store num::bigint in the LitInt type to pass the number to the lint stage.

[lilyball]

I think num::bigint might be overkill. Having a fixed-size "bigger than u64" type might be nice, but I think we can safely ignore that until we actually want to support integer literal conversion to arbitrary types. Until that point, I think it's ok to emit a parse error when consuming a literal that cannot be represented in the AST.

[hirschenberger]

What about this issue, any suggestions, hints, wishes?

[lilyball]

At first glance it looks ok to me, but I'd rather let someone else with more experience with the AST review it.

[hirschenberger]

@kballard Who would be appropriate?

[lilyball]

Not sure. @alexcrichton? @cmr?

[hirschenberger]

Ok, I fixed the compile error. Unfortunately Travis has problems with a test which unrelated to my changes. So I think it's ready to roll.