Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add AnonConst as potential parent_item #71477

Closed
wants to merge 2 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Apr 23, 2020

This prevents some help messages in const contexts, due to

if self.tcx.hir().is_const_context(expr.hir_id) {
// Shouldn't suggest `.into()` on `const`s.
// FIXME(estebank): modify once we decide to suggest `as` casts
return false;
}

considering that the check in is_const_context was previously unreachable,
this is actually a bugfix 😆
| Node::AnonConst(_) => true,

r? @eddyb cc @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2020
|
help: you can convert an `isize` to `usize` and panic if the converted value wouldn't fit
|
LL | let f = [0; (-4_isize).try_into().unwrap()];
Copy link
Contributor Author

@lcnr lcnr Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This help message was incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estebank
Copy link
Contributor

It'd be nice if we changed check_for_cast to still emit a suggestion for literal suffix changes. Would you mind doing that in a separate PR or filing a ticket after this is merged?

@lcnr
Copy link
Contributor Author

lcnr commented Apr 23, 2020

Can implement it in a separate PR 👍

@eddyb
Copy link
Member

eddyb commented Apr 30, 2020

I don't think this is the right approach, get_parent_item should barely exist itself, what's the actual ICE being fixed here? What's calling get_parent_item on something inside AnonConst and hoping to see the body owner? Because that's what it should be looking for, not "parent item".

@lcnr
Copy link
Contributor Author

lcnr commented Apr 30, 2020

This PR is needed for https://github.com/rust-lang/rust/blob/4b575996168edbc5b7ad46229f5ed3bfee35484c/src/test/ui/const-generics/type-dependent/const-arg-in-const-arg.rs #70 (#71154 uses it to get the correct typeck_tables)

get_parent_item is also used in is_const_context to get the body owner of an anon cost.

let parent_id = self.get_parent_item(hir_id);

which currently results in wrong diagnostics in

if self.tcx.hir().is_const_context(expr.hir_id) {
// Shouldn't suggest `.into()` on `const`s.
// FIXME(estebank): modify once we decide to suggest `as` casts
return false;
}
to get the body owner of an anon cost.

What do you want to use instead?

@eddyb
Copy link
Member

eddyb commented May 1, 2020

You want to loop through the parents with get_parent_node and stop when maybe_body_owned_by returns Some. That is the enclosing body owner, which is what you want.

If you want to be more efficient you could probably add an enclosing_body_owner(_def_id) which is like body_owner(_def_id) but does the loop I described above, stopping at a node for which is_body_owner returns true.

cc @ecstatic-morse @oli-obk @wesleywiser on is_const_context, I'm suspicious of its correctness and uses (it probably wants to be unified with whatever MIR const-checking uses)

@lcnr
Copy link
Contributor Author

lcnr commented May 1, 2020

I now use the following in #71154:

    pub fn enclosing_body_owner(&self, hir_id: HirId) -> HirId {
        for (parent, _) in self.parent_iter(hir_id) {
            if let Some(body) = self.maybe_body_owned_by(parent) {
                return self.body_owner(body);
            }
        }

        bug!("no `enclosing_body_owner` for hir_id `{}`", hir_id);
    }

This means that this PR now only fixes incorrect suggestions afaict. Don't fully understand when get_parent_node is appropriate yet, so I don't know how to proceed here.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2020
@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2020
@lcnr
Copy link
Contributor Author

lcnr commented May 19, 2020

Closing this for now as this is not the correct approach.

Will try and make a more general fix for is_const_context in the near future.

@lcnr lcnr closed this May 19, 2020
@lcnr lcnr deleted the get_parent_item branch May 19, 2020 21:14
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 11, 2020
Fix `is_const_context`, update `check_for_cast`

A better version of rust-lang#71477

Adds `fn enclosing_body_owner` and uses it in `is_const_context`.
`is_const_context` now uses the same mechanism as `mir_const_qualif` as it was previously incorrect.
Renames `is_const_context` to `is_inside_const_context`.

I also updated `check_for_cast` in the second commit, so r? @estebank

(I removed one lvl of indentation, so it might be easier to review by hiding whitespace changes)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 11, 2020
Fix `is_const_context`, update `check_for_cast`

A better version of rust-lang#71477

Adds `fn enclosing_body_owner` and uses it in `is_const_context`.
`is_const_context` now uses the same mechanism as `mir_const_qualif` as it was previously incorrect.
Renames `is_const_context` to `is_inside_const_context`.

I also updated `check_for_cast` in the second commit, so r? @estebank

(I removed one lvl of indentation, so it might be easier to review by hiding whitespace changes)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants