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

Properly enforce the "patterns aren't allowed in foreign functions" rule #35015

Merged
merged 2 commits into from
Aug 4, 2016

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jul 24, 2016

Cases like arg @ PATTERN or mut arg were missing.
Apply the same rule to function pointer types.

Closes #35203
[breaking-change], no breakage in sane code is expected though
r? @nikomatsakis

This is somewhat related to rust-lang/rfcs#1685 (cc @matklad).
The goal is to eventually support full pattern syntax where it makes sense (function body may present) and to support only the following forms - TYPE, ident: TYPE, _: TYPE - where patterns don't make sense (function body doesn't present), i.e. in foreign functions and function pointer types.

@nikomatsakis
Copy link
Contributor

Started crater run.

@nikomatsakis
Copy link
Contributor

PR looks good, but I'd like to see the results of the crater run so as to decide if this should be a warning or what.

@matklad
Copy link
Member

matklad commented Jul 26, 2016

Hm, if we make the plain TYPE syntax valid, then we perhaps be unable to give "You declared a pattern as an argument in a foreign function declaration." diagnostic, because we'll be unable to parse a pattern in that position.

@nikomatsakis
Copy link
Contributor

Crater run failed for some reason. =(

@nikomatsakis
Copy link
Contributor

Trying to restart this crater run.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 2, 2016

Argh, sorry, that took a while. Here was the result:

https://gist.github.com/nikomatsakis/47dea4e98ffa9d3f350838602cc2a080

5424 crates tested: 3824 working / 1501 broken / 6 regressed / 35 fixed / 58 unknown.

Of those 6 regressions, only commands-0.0.3 was definitely caused by this PR. simd-0.11 was a timeout and hence inconclusive. The others all seemed to have to do with serde in some way that did not seem particularly related to this PR (confusing).

So, per rust-lang/rfcs#1589, So I'd be happy to land this change without a lint warning, but we are still missing a few things:

  • A special note on the change indicating that it is a new error and redirecting people to a tracking issue
  • A tracking issue explaining what changed
  • Submit a PR to commands-0.0.3

…heck

Apply the same check to function pointer types
@petrochenkov
Copy link
Contributor Author

@nikomatsakis
Done.

@nikomatsakis
Copy link
Contributor

@petrochenkov great!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 3, 2016

📌 Commit 5c88efc has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 4, 2016

⌛ Testing commit 5c88efc with merge 271d048...

bors added a commit that referenced this pull request Aug 4, 2016
Properly enforce the "patterns aren't allowed in foreign functions" rule

Cases like `arg @ PATTERN` or `mut arg` were missing.
Apply the same rule to function pointer types.

Closes #35203
[breaking-change], no breakage in sane code is expected though
r? @nikomatsakis

This is somewhat related to rust-lang/rfcs#1685 (cc @matklad).
The goal is to eventually support full pattern syntax where it makes sense (function body may present) and to support *only* the following forms - `TYPE`, `ident: TYPE`, `_: TYPE` - where patterns don't make sense (function body doesn't present), i.e. in foreign functions and function pointer types.
@bors bors merged commit 5c88efc into rust-lang:master Aug 4, 2016
@petrochenkov petrochenkov deleted the forearg branch September 21, 2016 20:01
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 26, 2016
Prohibit patterns in trait methods without bodies

They are not properly type checked
```rust
trait Tr {
    fn f(&a: u8); // <- This compiles
}
```
, mostly rejected by the parser already and generally don't make much sense.
This PR is kind of a missing part of rust-lang#35015.

Needs crater run.
cc rust-lang#35078 (comment) rust-lang#35015 rust-lang/rfcs#1685 rust-lang#35203
r? @eddyb
bors added a commit that referenced this pull request Oct 29, 2016
Prohibit patterns in trait methods without bodies

They are not properly type checked
```rust
trait Tr {
    fn f(&a: u8); // <- This compiles
}
```
, mostly rejected by the parser already and generally don't make much sense.
This PR is kind of a missing part of #35015.

Given the [statistics from crater](#37378 (comment)), the effect of this PR is mostly equivalent to improving `unused_mut` lint.

cc #35078 (comment) #35015 rust-lang/rfcs#1685 #35203
r? @eddyb
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