Removed the promotable field from CheckCrateVisitor... #52318
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
ping @eddyb probably of interest to you I need this for being able to do #51570 (comment) properly. It was next to impossible to do with the mutable state present. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
eddyb
reviewed
Jul 12, 2018
| struct Promotable; | ||
|
|
||
| struct NotPromotable; | ||
|
|
||
| impl<'a, 'gcx> CheckCrateVisitor<'a, 'gcx> { | ||
| // Returns true iff all the values of the type are promotable. | ||
| fn type_has_only_promotable_values(&mut self, ty: Ty<'gcx>) -> bool { |
There was a problem hiding this comment.
This should also return Result, I'm guessing.
eddyb
reviewed
Jul 12, 2018
| impl<'a, 'gcx> CheckCrateVisitor<'a, 'gcx> { | ||
| // Returns true iff all the values of the type are promotable. | ||
| fn type_has_only_promotable_values(&mut self, ty: Ty<'gcx>) -> bool { | ||
| debug!("In type_has_only_promotable_values(), ty: {:#?}", ty); |
There was a problem hiding this comment.
The typical style for this is more like:
debug!("type_has_only_promotable_values({})", ty);
eddyb
reviewed
Jul 12, 2018
| fn handle_const_fn_call(&mut self, def_id: DefId, | ||
| ret_ty: Ty<'gcx>, span: Span) -> Result<Promotable, NotPromotable> { | ||
| debug!("In handle_const_fn_call(), def_id: {:#?}, span: {:#?}, ret_ty: {:#?}", | ||
| def_id, span, ret_ty); |
There was a problem hiding this comment.
Same here. Also, please don't use {:#?} unless necessary, it makes debug logs harder to work with. Also, for Ty you only need {}, not even {:?}.
There was a problem hiding this comment.
That one's on me. The logs were unreadable in the unexpanded version (well not for types, but everything else) I wish we had a way to switch the style with an env var
There was a problem hiding this comment.
That's weird. Neither Span nor DefId should be doing anything with {:#?}. Maybe you mean the AST? But I wouldn't print ASTs at all...
There was a problem hiding this comment.
Yea the ASTs. They were pretty helpful during debugging.
There was a problem hiding this comment.
cc @nikomatsakis We've avoided printing ASTs in debug! before, and a lot of times they are useless (i.e. printing NodeIds with --unpretty=hir,identified can be a better debugging experience), but maybe I'm wrong.
eddyb
reviewed
Jul 12, 2018
| @@ -159,6 +179,8 @@ impl<'a, 'gcx> CheckCrateVisitor<'a, 'gcx> { | |||
| /// and transfer it over to the value being matched. This will | |||
| /// then prevent said value from being promoted. | |||
| fn remove_mut_rvalue_borrow(&mut self, pat: &hir::Pat) -> bool { | |||
| debug!("In remove_mut_rvalue_borrow(), pat: {:#?}", pat); | |||
There was a problem hiding this comment.
Same here and everywhere in the file, I guess. (it's always a good idea to copy the style from somewhere else in the compiler)
eddyb
reviewed
Jul 12, 2018
| @@ -170,6 +192,8 @@ impl<'a, 'gcx> CheckCrateVisitor<'a, 'gcx> { | |||
|
|
|||
| impl<'a, 'tcx> CheckCrateVisitor<'a, 'tcx> { | |||
There was a problem hiding this comment.
Since when is this not a visitor impl, huh?!
eddyb
reviewed
Jul 12, 2018
| self.in_fn = outer_in_fn; | ||
| self.tables = outer_tables; | ||
| self.param_env = outer_param_env; | ||
| self.identity_substs = outer_identity_substs; | ||
| } | ||
|
|
||
| fn visit_stmt(&mut self, stmt: &'tcx hir::Stmt) { | ||
| fn visit_stmt(&mut self, stmt: &'tcx hir::Stmt) -> Result<Promotable, NotPromotable> { |
There was a problem hiding this comment.
If you're going to change the signatures of these functions, I'd also change the names to check_* instead of visit_*.
eddyb
reviewed
Jul 12, 2018
| } | ||
| _ => Err(NotPromotable), | ||
| }; | ||
| def_result.and(call_result) |
There was a problem hiding this comment.
You don't need to do anything like this, you can return early at the first sign of Err.
There was a problem hiding this comment.
All visit_* calls need to happen, otherwise if e.g. the first function argument is not promotable, the next one isn't going to be inserted into the promotable map, even if it were promotable
In this case we can early abort right after the for loop though
There was a problem hiding this comment.
Oh, right, oops, you're right. Why are you even using Result then though? I'm guessing for clarity?
There was a problem hiding this comment.
Yea we could move to a custom enum. Do you prefer that? At the beginning I thought the same thing wrt early return, but it's actually pretty easy to screw up, so maybe a custom enum would be better
There was a problem hiding this comment.
Yupp! Frankly I'm happing with a boolean but those have && and || so a custom enum that implements BitAnd is probably the best option.
oli-obk
requested changes
Jul 13, 2018
| } | ||
|
|
||
| fn handle_const_fn_call(&mut self, def_id: DefId, | ||
| ret_ty: Ty<'gcx>, span: Span) -> Promotability { | ||
| debug!("In handle_const_fn_call(), def_id: {:#?}, span: {:#?}, ret_ty: {:#?}", | ||
| def_id, span, ret_ty); | ||
|
|
||
| if !self.type_has_only_promotable_values(ret_ty) { return NotPromotable; } | ||
| let promotability_output = self.type_promotability(ret_ty); | ||
| if let Promotable = promotability_output { |
There was a problem hiding this comment.
remove this if let and just & the promotability_output into the other variables at the end of this function
| return NotPromotable | ||
| let promotability_output = v.type_promotability(node_ty); | ||
| if !promotable { | ||
| return promotability_output; |
There was a problem hiding this comment.
remove the promotable variable entirely and return NotPromotable` in the appropriate places in the if condition above
| return NotPromotable | ||
| let promotability_output = v.type_promotability(node_ty); | ||
| if !promotable { | ||
| return promotability_output; | ||
| } | ||
| Promotable |
There was a problem hiding this comment.
remove this and just place the v.type_promotability(node_ty) call here and let the value fall through
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
oli-obk
approved these changes
Jul 14, 2018
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
oli-obk
requested changes
Jul 14, 2018
| } | ||
|
|
||
| fn visit_block(&mut self, block: &'tcx hir::Block) { | ||
| fn check_block(&mut self, block: &'tcx hir::Block) -> Promotability { | ||
| debug!("e.node: {:#?}", block); |
There was a problem hiding this comment.
Remove this AST-printing message
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
…ith the structs Promotable and NotPromotable.
oli-obk
approved these changes
Jul 14, 2018
|
@bors r+ |
|
📌 Commit 07faca9 has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jul 14, 2018
Removed the promotable field from CheckCrateVisitor... and replaced it with the custom enum Promotability. r? @oli-obk
|
☀️ Test successful - status-appveyor, status-travis |
eddyb
reviewed
Jul 15, 2018
| mut_rvalue_borrows: NodeSet, | ||
| param_env: ty::ParamEnv<'tcx>, | ||
| identity_substs: &'tcx Substs<'tcx>, | ||
| tables: &'a ty::TypeckTables<'tcx>, | ||
| result: ItemLocalSet, | ||
| } | ||
|
|
||
| #[must_use] | ||
| #[derive(Debug, PartialEq)] | ||
| enum Promotability { |
There was a problem hiding this comment.
Maybe this should be exported and used instead of bool elsewhere in the compiler.
There was a problem hiding this comment.
Are there any other things I'd need to implement (like BitAnd and BitOr) to cover the rest of the compiler?
There was a problem hiding this comment.
I think BitOr should be removed, but otherwise you'd likely just have to try and see.
eddyb
reviewed
Jul 15, 2018
|
|
||
| _ => { | ||
| v.promotable = false; | ||
| promotable | v.type_promotability(node_ty) |
There was a problem hiding this comment.
This is the only place | is used, since misusable. Also, the method was named type_has_only_promotable_values before, which suggests it's impossible to create an unpromotable value of that type. type_promotability is a bit more general but it does highlight a point: this code is probably wrong.
@oli-obk this should probably not be relaxed like this. You might need to reintroduced bitflags to indicate the reason for unpromotability Q_Q. (i.e. you can remove INTERIOR_MUT and NEEDS_DROP flags based on the type but you can't remove anything pertaining to "creates values that cannot be operated on correctly").
There was a problem hiding this comment.
I have a feeling we discussed this before and it ended up being a breaking change to change this.
"has only promotable values" made sense pre-miri. Not so much afterwards.
This is pretty much the topic I'm addressing in #51570
There was a problem hiding this comment.
I so wish we could just throw away the old borrow-checker and avoid all the lingering complexity...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
and replaced it with the custom enum Promotability.
r? @oli-obk