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

Updated clippy to account for changes from rust-lang/rust#44766 #2140

Merged
merged 1 commit into from
Oct 30, 2017
Merged

Updated clippy to account for changes from rust-lang/rust#44766 #2140

merged 1 commit into from
Oct 30, 2017

Conversation

sunjay
Copy link
Member

@sunjay sunjay commented Oct 15, 2017

In rust-lang/rust#44766, I make some changes to the ast and the hir which break some clippy code. Please DO NOT merge these changes until that PR is merged. This PR shouldn't even pass CI until those changes land in the compiler, but I thought I would mention it anyway just in case.

Some of the breaking changes were easy to fix, but there is one particular lint that I don't know how to fix. There's a TODO comment and commented out code in this PR to show where the problem is. I would appreciate some advice about how to go about fixing this.

The problem is that there is no more sig.generics. As part of my rustc PR, I lifted the generics property from each individual method signature to the trait/impl item itself. This is in preparation for implementing generic associated types. When I fixed rustfmt, I was able to resolve this by adding a parameter to the function that needed to check generics so that the generics could be passed in from above. I tried doing that here but ran into some problems along the way. Do you think the right thing to do is to add a generics parameter to check_fn?

@Manishearth
Copy link
Member

The way to fix that is to change the linting API to also pass down an &Generics for all functions. check_fn in rustc is no longer correct because it does not pass down all the information of a function anymore.

@sunjay
Copy link
Member Author

sunjay commented Oct 15, 2017

@Manishearth Thanks for the quick reply. Yes that's exactly what I suspected (and what I wrote in my PR description). I'll figure it out and update everything.

Thanks for taking a look! :)

@Manishearth
Copy link
Member

Make sure that before merging the rust side the clippy commit you point to is within the clippy repo.

I can do this for you once you're sure you're ready to merge (until then, wait).

This way the references in rust history never break.

@sunjay
Copy link
Member Author

sunjay commented Oct 15, 2017

Sorry, I should have added more details. The exact process to be followed is in this comment. Here's the relevant bit:

A maintainer of clippy will not merge the PR. The PR can't be merged because CI will be broken. Instead a new branch will be created, and the PR will be pushed to the branch (the PR is left open)

@Manishearth
Copy link
Member

Yep, I'm aware. I'm saying I'll do that when you're sure it's ready to be merged.

@sunjay
Copy link
Member Author

sunjay commented Oct 15, 2017

Perfect! Thank you!

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 16, 2017

In my opinion, the correct fix is not to thread down hir::Generics. The idea is that this is being visited at an outer level, consistently (same as it is today for e.g. closures).

Rather, we should extend the LateContext struct to include a ty::Generics field like so:

/// Context for lint checking after type checking.
pub struct LateContext<'a, 'tcx: 'a> {
    /// Type context we're checking in.
    pub tcx: TyCtxt<'a, 'tcx, 'tcx>,

    /// Side-tables for the body we are in.
    pub tables: &'a ty::TypeckTables<'tcx>,

    /// Parameter environment for the item we are in.
    pub param_env: ty::ParamEnv<'tcx>,

    /// Generic type parameters in scope for the item we are in.
    pub generics: Option<&'tcx ty::Generics>, // <-- THIS IS NEW

    /// Items accessible from the crate being checked.
    pub access_levels: &'a AccessLevels,

    /// The store of registered lints and the lint levels.
    lint_sess: LintSession<'tcx, LateLintPassObject>,

    last_ast_node_with_lint_attrs: ast::NodeId,
}

I used an Option because there doesn't seem to be an obvious "empty" generics value to use.

(To be honest, I think that typeck_tables and param_env probably ought to be Option as well -- right now, they are initialized to some empty values when we are outside of any item, but it feels like they should be None in that case. Not that I would change this right now.)

We can then initialize the field to None and then update it in with_param_env, roughly like:

let old_generics = self.generics;
self.generics = self.tcx.generics_of(self.tcx.hir.local_def_id(id));
...
self.generics = old_generics;

Now we can change the lint code in NewWithoutDefault to:

if !cx.generics.unwrap().types.is_empty() {
    // when the result of `new()` depends on a type parameter we should not require
    // an impl of `Default`
    return;
}

@sunjay
Copy link
Member Author

sunjay commented Oct 16, 2017

I used an Option because there doesn't seem to be an obvious "empty" generics value to use.

What about Generics::default()?

@Manishearth
Copy link
Member

Ah, sticking generics on the late context is a good idea.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 28, 2017

I won't get to a PC until Monday. Can someone else take this PR?

@Manishearth
Copy link
Member

on it

@sunjay
Copy link
Member Author

sunjay commented Oct 28, 2017

Hi Manish, if you'd like me to go in and finish this I'm happy to do it. It sounds like you're already working on it currently so I'll leave it to you. I wouldn't want our work to conflict. :)

@Manishearth
Copy link
Member

Pushed to the rustup branch on this repo. I couldn't find the Generics field on LateContext and need to head out in a bit, so left that undone.

@Manishearth
Copy link
Member

It seems like LateContext was never changed to provide a Generics field? Is there an alternate way to handle this? Or is this something that was supposed to be added to rustc itself?

@sunjay
Copy link
Member Author

sunjay commented Oct 28, 2017

I believe the idea was to change LateContext to include that field. Niko wrote "THIS IS NEW" beside it in his instructions.

I may have some time to look into this a bit later today or tomorrow. I'll be able to help you more then. :)

@Manishearth
Copy link
Member

Oh, I thought the plan was for you to include that in your Rust PR.

I've got a WIP of it here: https://github.com/manishearth/rust/tree/lint-generics . It should work but I haven't compiled it yet (and will be getting on a flight so won't be able to look at it until later).

@sunjay
Copy link
Member Author

sunjay commented Oct 29, 2017

Manish has incorporated my commit into #2187. When that is merged I think this can be closed. Thanks Manish! :)

bors added a commit to rust-lang/rust that referenced this pull request Oct 29, 2017
Add generics to LateContext

Fixes clippy breakage from #44766 as discussed in rust-lang/rust-clippy#2140 (comment)

r? @nikomatsakis
@Manishearth Manishearth merged commit da14435 into rust-lang:master Oct 30, 2017
@sunjay sunjay deleted the lift_generics branch October 30, 2017 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants