Don't crash when checking types with only region parameters for destructor safety #15585
Conversation
| try!(write!(fmt, "}}")); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
rust conventions are to use four-space tabs instead of 2
There was a problem hiding this comment.
Doh, sorry about that. I had my editor configured to work on Zinc while working this.
|
Can you add a test for the fixed issue as well? |
|
Sure! |
| format!("Type parameter out of range \ | ||
| when substituting in region {} (root type={})", | ||
| region_name.as_str(), | ||
| self.root_ty.repr(self.tcx())).as_slice()); |
There was a problem hiding this comment.
@alexcrichton is the two-space indentation here acceptable?
There was a problem hiding this comment.
The formatting here would generally look like:
format!("foo bar baz \
aligned after quote",
aligned_with_quote);
|
@alexcrichton everything should be fixed other than the test. Where should the test go? |
|
@bgamari I think (FYI: I think |
|
@bgamari also, when you are inevitably asked to squash your commtis together before they are finally merged, I would request that you try to keep the nice refactorings (e.g. the additions of (The above may be an obvious thing to do, but not everyone takes the time to squash things into more than one commit, so that is why I'm explicitly asking.) |
|
FWIW, |
|
@pnkfelix how does this patch structure look? |
|
Pushed a rebased set. Seems to compile on my end and I believe the failing Travis build isn't my fault. |
|
@bgamari please squash fca9dec into e149dc9 |
|
@pnkfelix oops, that had been on my todo list. Thanks for the reminder. |
|
@pnkfelix this is ready, by the way. |
|
Alright, here we go again. |
And change some uses of the `{:?}` format string to `{}`.
Previously this was an Option::unwrap() which failed for me. Unfortunately I've since inadvertently worked around the bug and have been unable to reproduce it. With this patch hopefully the next person to encounter this will be in a slightly better position to debug it.
To verify that a type can satisfy Send `check_struct_safe_for_destructor` attempts to construct a new `ty::t` an empty substitution list. Previously the function would verify that the function has no type parameters before attempting this. Unfortunately this check would not catch functions with only regions parameters. In this case, the type would eventually find its way to the substition engine which would attempt to perform a substitution on the region parameters. As the constructed substitution list is empty, this would fail, leading to a compiler crash. We fix this by verifying that types have both no type and region parameters.
|
Updated and travis passed. |
This branch has a fix for #15557 (a2bcef9) as well as a variety of patches I found useful while debugging this issue. These include adding `Show` impls to a variety of types, including the majority of `syntax::ast` and some of `middle::ty`.
This branch has a fix for #15557 (a2bcef9) as well as a variety of patches I found useful while debugging this issue. These include adding
Showimpls to a variety of types, including the majority ofsyntax::astand some ofmiddle::ty.