Skip to content

Work around warning from late_bound_lifetime_arguments lint #260

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

Merged
merged 1 commit into from
Oct 21, 2019
Merged

Work around warning from late_bound_lifetime_arguments lint #260

merged 1 commit into from
Oct 21, 2019

Conversation

lqd
Copy link
Contributor

@lqd lqd commented Oct 15, 2019

The generic parsing functions require lifetime annotations to build because of rust-lang/rust#42508.

However, this creates a warning from the late_bound_lifetime_arguments compatibility lint, described in rust-lang/rust#42868, because they have both early-bound and late-bound lifetime parameters.

This PR works around this warning, in a functional but sub-optimal way. The result is quite verbose, and I didn't manage to "convince" rustc to think all lifetimes were early-bound at the fn definitions, but manages to avoid triggering both rust-lang/rust#42508 and the lint, by passing these closures via variables at each call-site.

Fixes #254

@lqd
Copy link
Contributor Author

lqd commented Oct 15, 2019

(added a scope to make it build on 1.31, this is not required for the current stable)

@emilio
Copy link
Member

emilio commented Oct 15, 2019

Is there any good reason to do this other than building warning-free?

I don't think we should adapt your code for this necessarily, unless there's a very good reason to do this. Doubly so if this makes the code uglier tbf...

@lqd
Copy link
Contributor Author

lqd commented Oct 16, 2019

Yes building without the warning was the reason for the change, as it's not clear when this is going to become an error.

@emilio
Copy link
Member

emilio commented Oct 16, 2019

As long as this is working around a rust bug I don't think they can reasonably turn it into an error...

@lqd
Copy link
Contributor Author

lqd commented Oct 16, 2019

Ah but this PR is not working around a rustc bug, it is "fixing" rust-cssparser's workaround for the rustc bug and which generates a compatiblity lint warning.

(the code is not that ugly on current stable. I haven't tested older stables so I don't know which one fixed the scope requirement, I only added it because CI fails on 1.31. These, and the existing ones, eg in parser.rs IIRC, could be removed on newer versions)

@SimonSapin
Copy link
Member

The code comment is useful, but perhaps the whole explanation does not need to be repeated every time. Maybe move it to a github comment in #254, and have the code link there?

With that change I feel the ugliness would not be too bad. I think warning-freedom is valuable, although in this case we aren’t actively doing a lot of work on this crate so warnings haven’t been much of a bother.

@lqd lqd mentioned this pull request Oct 16, 2019
@SimonSapin
Copy link
Member

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit d5006f6 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

⌛ Testing commit d5006f6 with merge 0c9c54b...

bors-servo pushed a commit that referenced this pull request Oct 21, 2019
Work around warning from `late_bound_lifetime_arguments` lint

The generic parsing functions require lifetime annotations to build because of rust-lang/rust#42508.

However, this creates a warning from the `late_bound_lifetime_arguments` compatibility lint, described in rust-lang/rust#42868, because they have both early-bound and late-bound lifetime parameters.

This PR works around this warning, in a functional but sub-optimal way. The result is quite verbose, and I didn't manage to "convince" rustc to think all lifetimes were early-bound at the `fn` definitions, but manages to avoid triggering both rust-lang/rust#42508 and the lint, by passing these closures via variables at each call-site.

Fixes #254
@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: SimonSapin
Pushing 0c9c54b to master...

@bors-servo bors-servo merged commit d5006f6 into servo:master Oct 21, 2019
@lqd lqd deleted the warning_begone branch October 21, 2019 11:52
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.

Future compat warning
4 participants