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

Partially revert the early feature-gatings added in #65742. #66004

Merged
merged 12 commits into from
Nov 2, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Oct 31, 2019

The intent here is to address #65860 ASAP (in time for beta, ideally), while leaving as much of #65742 around as possible, to make it easier to re-enable later.

Therefore, I've only kept the parts of the revert that re-add the old (i.e. non-early) feature-gating checks that were removed in #65742, and the test reverts.

I've disabled the new early feature-gating checks from #65742 entirely for now, but it would be easy to put them behind a -Z flag, or turn them into warnings, which would allow us to keep tests for both the early and late versions of the checks - assuming that's desirable.

cc @nikomatsakis @Mark-Simulacrum @Centril

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov
Copy link
Contributor

@eddyb
Could you summarize the discussion that happened about this during the week?
Was the decision to revert until some RFC is written or something else?

I think #65742 did the right thing, but if any specific gate causes regressions/issues in practice we can revert it or turn into a soft_unstable lint, which is preferable to reverting everything.

@petrochenkov petrochenkov 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 Nov 1, 2019
@Mark-Simulacrum
Copy link
Member

We wanted to revert so that at least this hits nightly early in the cycle rather than in the last week for additional testing and also wanted some discussion around the general design (in particular, I feel like this is too strict -- I would like to see some exploration of allowing cfg's on stable syntax that may contain unstable syntax).

We also felt that the existing crater run was likely not hitting all the cases, specifically, since it was a nightly-enabled crater run, that meant that crates may have been detecting as such and turning features on (which would've masked errors that the PR introduced otherwise). I've been put on the hook for helping out with that crater run, and was planning on trying to do so this weekend.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2019
@petrochenkov
Copy link
Contributor

Understood.
@bors r+

@bors
Copy link
Contributor

bors commented Nov 2, 2019

📌 Commit 27e01a1 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2019
@petrochenkov
Copy link
Contributor

@bors p=100
Needs to be merged before beta.

@bors
Copy link
Contributor

bors commented Nov 2, 2019

⌛ Testing commit 27e01a1 with merge f39205b...

bors added a commit that referenced this pull request Nov 2, 2019
Partially revert the early feature-gatings added in #65742.

The intent here is to address #65860 ASAP (in time for beta, ideally), while leaving as much of #65742 around as possible, to make it easier to re-enable later.

Therefore, I've only kept the parts of the revert that re-add the old (i.e. non-early) feature-gating checks that were removed in #65742, and the test reverts.

I've disabled the new early feature-gating checks from #65742 entirely for now, but it would be easy to put them behind a `-Z` flag, or turn them into warnings, which would allow us to keep tests for both the early and late versions of the checks - assuming that's desirable.

cc @nikomatsakis @Mark-Simulacrum @Centril
@bors
Copy link
Contributor

bors commented Nov 2, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing f39205b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 2, 2019
@bors bors merged commit 27e01a1 into rust-lang:master Nov 2, 2019
@eddyb eddyb deleted the revert-early-gate branch November 2, 2019 14:14
@est31 est31 mentioned this pull request Feb 26, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 11, 2023
…-loc, r=compiler-errors

Lint against misplaced where-clauses on associated types in traits

Extends the scope of the lint `deprecated_where_clause_location` (rust-lang#89122) from associated types in impls to associated types in any location (impl or trait). This is only relevant for `#![feature(associated_type_defaults)]`. Previously we didn't warn on the following code for example:

```rs
#![feature(associated_type_defaults)]
trait Trait { type Assoc where u32: Copy = (); }
```

Personally I would've preferred to emit a *hard* error here instead of a lint warning since the feature is unstable but unfortunately we are constrained by back compat as associated type defaults won't necessarily trigger the feature-gate error if they are inside of a macro call (since they use a post-expansion feature-gate due to historical reasons, see also rust-lang#66004).

I've renamed and moved related preexisting tests: 1. They test AST validation passes not the parser & thus shouldn't live in `parser/` (historical reasons?). 2. One test file was named after type aliases even though it tests assoc tys.

`@rustbot` label A-lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants