Don't allow underflow when calculating spans #35990
Conversation
This subtraction can take place when doing things like creating a [field access expression through the AST builder][builder field access] (and in fact, as best I can tell, that's the only place it occurs) [builder field access]: https://github.com/rust-lang/rust/blob/a5e5ea1646367b82864af3a2a508993d76b792af/src/libsyntax/ext/build.rs#L636-L645 The assumption is that the given span starts and ends a the last byte of the field access. However, when doing macro expansion such as custom derive (or any other place the AST builder would manually be used), it's quite common to use the site of expansion as the span, as there's no other printable span to show. If that macro expansion is the first line of a file, the compiler will panic from underflow. Since byte positions are unsized, and performing this subtraction in a situation for underflow pretty much universally means you want to remain at 0, I've fixed this in the sub impl rather than the two problem cases of the AST builder.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| @@ -549,7 +549,8 @@ impl Sub for BytePos { | |||
| type Output = BytePos; | |||
|
|
|||
| fn sub(self, rhs: BytePos) -> BytePos { | |||
| BytePos((self.to_usize() - rhs.to_usize()) as u32) | |||
| let new_pos = self.to_usize().checked_sub(rhs.to_usize()).unwrap_or(0); | |||
| @@ -549,7 +549,8 @@ impl Sub for BytePos { | |||
| type Output = BytePos; | |||
|
|
|||
| fn sub(self, rhs: BytePos) -> BytePos { | |||
| BytePos((self.to_usize() - rhs.to_usize()) as u32) | |||
| let new_pos = self.to_usize().saturating_sub(rhs.to_usize()); | |||
| BytePos(new_post as u32) | |||
|
The span mangling in fn expr_field_access(&self, sp: Span, expr: P<ast::Expr>, ident: ast::Ident) -> P<ast::Expr> {
let field_span = Span {
lo: sp.lo - Pos::from_usize(ident.name.as_str().len()),
hi: sp.hi,
expn_id: sp.expn_id,
};
// ...
}It does not feel like it will create the correct span even in the "macro-free" (e.g. I would the correct span to be cc @nrc |
|
I agree that it felt incorrect for that span to be modified at all, but I was unsure if there was a reason or not so this felt like the more conservative change. I would be fine with changing the offending code to use the passed span instead. |
|
r? @nrc |
|
So, I think @arielb1 is right on both points - the span calculation is wrong, and So, I'm a bit reluctant to merge this PR - it feels like we should never hit this situation, and if we overflow then it is always because the caller screwed up, and therefore panicking is the correct response. On the other hand, that is not very defensive programming, so maybe it is better to have this than not. I'll ponder a bit. In any case, thanks @sgrif and @arielb1 for finding this issue and digging into it. |
|
If we're fine with dumping the span mangling this PR is unnecessary IMO. Happy to update to dump the span mangling instead. |
|
@sgrif it would be great if you wanted up update the span mangling. Given that we agree that that is the way forward, I'll close this PR. |
This subtraction can take place when doing things like creating a field
access expression through the AST builder (and in
fact, as best I can tell, that's the only place it occurs)
The assumption is that the given span starts and ends a the last byte of
the field access. However, when doing macro expansion such as custom
derive (or any other place the AST builder would manually be used), it's
quite common to use the site of expansion as the span, as there's no
other printable span to show. If that macro expansion is the first line
of a file, the compiler will panic from underflow.
Since byte positions are unsized, and performing this subtraction in a
situation for underflow pretty much universally means you want to remain
at 0, I've fixed this in the sub impl rather than the two problem cases
of the AST builder.