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

Unused type parameter error regression with nightly-2016-08-28 #36075

Closed
nox opened this issue Aug 28, 2016 · 6 comments
Closed

Unused type parameter error regression with nightly-2016-08-28 #36075

nox opened this issue Aug 28, 2016 · 6 comments
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@nox
Copy link
Contributor

nox commented Aug 28, 2016

The following code now fails to typecheck in latest nightly:

/// Provides an iterator for declaration list parsing.
pub struct DeclarationListParser<'i: 't, 't: 'a, 'a, I, P>
where P: DeclarationParser<Declaration = I> + AtRuleParser<AtRule = I> {
    /// The input given to `DeclarationListParser::new`
    pub input: &'a mut Parser<'i, 't>,

    /// The parser given to `DeclarationListParser::new`
    pub parser: P,
}

/// Provides an iterator for rule list parsing.
pub struct RuleListParser<'i: 't, 't: 'a, 'a, R, P>
where P: QualifiedRuleParser<QualifiedRule = R> + AtRuleParser<AtRule = R> {
    /// The input given to `RuleListParser::new`
    pub input: &'a mut Parser<'i, 't>,

    /// The parser given to `RuleListParser::new`
    pub parser: P,

    is_stylesheet: bool,
    any_rule_so_far: bool,
}
error[E0392]: parameter `I` is never used
   --> /Users/nox/src/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/cssparser-0.5.7/src/rules_and_declarations.rs:188:54
    |
188 | pub struct DeclarationListParser<'i: 't, 't: 'a, 'a, I, P>
    |                                                      ^ unused type parameter
    |
    = help: consider removing `I` or using a marker such as `std::marker::PhantomData`

error[E0392]: parameter `R` is never used
   --> /Users/nox/src/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/cssparser-0.5.7/src/rules_and_declarations.rs:258:47
    |
258 | pub struct RuleListParser<'i: 't, 't: 'a, 'a, R, P>
    |                                               ^ unused type parameter
    |
    = help: consider removing `R` or using a marker such as `std::marker::PhantomData`

error: aborting due to 2 previous errors

source

@SimonSapin
Copy link
Contributor

Note that this is a regression from stable. This code may be wrong, but the end users of an application that depends on a library that depends on a library that depends on a old version of the cssparser library won’t care about that when their app breaks after updating their compiler.

We’ve publish a new version of cssparser that removes the type parameter, but since this is a public type we’ve used a semver-breaking version number (0.5.6 → 0.6.0) and dependents won’t get it automatically. (Maybe we should have gone against semver in this case? I don’t know.)

This is somewhat unusual code, so I don’t know if anything other than cssparser is affected. It may be worth doing a Crater run.

It can we do end up keeping this language change/breakage, would a deprecation warning help? Or would Cargo suppress it when building something that (recursively) depends on on an old cssparser version?

@Aatch Aatch added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Aug 29, 2016
@SimonSapin
Copy link
Contributor

To clarify:

  • Building this code on 1.11.0 stable, 1.12.0-beta.2, or 1.13.0-nightly (1987131 2016-08-26) shows no warning. It errors on 1.13.0-nightly (a23064a 2016-08-27).
  • By Cargo supressing warnings, I mean it passing --cap-lints allow to rustc when building dependencies

@eddyb
Copy link
Member

eddyb commented Aug 29, 2016

Looks like it was accidentally caused by #36002. Sorry for the earlier dismissal, I didn't realize I isn't used as a type parameter, but only as an associated type. For example, DeclarationListParser could use:

where P: AtRuleParser + DeclarationParser<Declaration = <P as AtRuleParser>::AtRule>
// perhaps even just:
where P: AtRuleParser + DeclarationParser<Declaration = P::AtRule>

So it is possible it was correctly considered constrained before, but I'd want confirmation from @nikomatsakis or @arielb1 before changing anything.
We didn't have any test on this, which is worrisome either way.

@SimonSapin
Copy link
Contributor

In cssparser 0.6.0 I’ve removed where clauses on structs entirely, together with the type parameter. But I kept this extra type parameter on impls, for example:

-impl<'i, 't, 'a, I, P> Iterator for DeclarationListParser<'i, 't, 'a, I, P>
+impl<'i, 't, 'a, I, P> Iterator for DeclarationListParser<'i, 't, 'a, P>
 where P: DeclarationParser<Declaration = I> + AtRuleParser<AtRule = I> {
     type Item = Result<I, Range<SourcePosition>>;

(because using <P as AtRuleParser>::AtRule instead of I in multiple places like Item was more verbose, and I mildly disliked the asymmetry in the where clauses.)

Is this likely to be rejected later?

@arielb1
Copy link
Contributor

arielb1 commented Aug 29, 2016

Will investigate.

@nikomatsakis
Copy link
Contributor

@eddyb the intention is that yes you can have type parameters which are constrained by an associated type. I thought there were tests for this but perhaps not this particular scenario. I see @arielb1 has an open PR, however, will take a look now.

bors added a commit that referenced this issue Sep 3, 2016
fix broken type parameter indexing logic in wfcheck

r? @eddyb

Fixes #36075
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

No branches or pull requests

6 participants