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

libsyntax: Forbid type parameters in tuple indices #19211

Merged
merged 3 commits into from
Nov 23, 2014
Merged

libsyntax: Forbid type parameters in tuple indices #19211

merged 3 commits into from
Nov 23, 2014

Conversation

aochagavia
Copy link
Contributor

This breaks code like

let t = (42i, 42i);
... t.0::<int> ...;

Change this code to not contain an unused type parameter. For example:

let t = (42i, 42i);
... t.0 ...;

Closes #19096

[breaking-change]

r? @aturon

@aochagavia
Copy link
Contributor Author

Some questions:

  • Why do ExprTupField and ExprField have a Vec<P<Ty>>? They never take generic parameters... I think it would be better to remove it.
  • Trying to pass type parameters to a tuple index gives a parse error which is unrelated to tuple indices. Does it make sense to have a compile-fail test expecting a specific error? This means that future unrelated changes to the parser would require the error message in this test to be updated.

@ghost
Copy link

ghost commented Nov 22, 2014

@aochagavia Thanks for the PR! I think it would be nice if the error message was more targeted. For example see this PR: https://github.com/rust-lang/rust/pull/18879/files, which made an equivalent change to field expressions.

@ghost
Copy link

ghost commented Nov 22, 2014

@aochagavia To answer your first question, yes, you're right, feel free to remove them. :)

@aochagavia
Copy link
Contributor Author

@jakub- I have some questions regarding the error message. To get a more targeted one, there are two approaches I can think of:

  • Try to parse type parameters: a big drawback of this approach is that if you type t.0::; you get strange errors, like expected '<', found ';'
  • Try to parse only a '::'. In case of success, show an error like "unexpected token '::'" and a note saying "tuple indices may not have type parameters". I think this is the one to go...

@aochagavia
Copy link
Contributor Author

I have implemented the second approach... But it is not a perfect solution and it can be confusing as well:

test.rs:5:23: 5:25 error: unexpected `::` after tuple index
test.rs:5     println!("{}", t.0::<Vec<uint>>)
                                ^~
test.rs:5:23: 5:25 note: tuple indices may not have type parameters
test.rs:5     println!("{}", t.0::<Vec<uint>>)
                                ^~
test.rs:5:34: 5:36 error: unexpected token: `<eof>`
test.rs:5     println!("{}", t.0::<Vec<uint>>)
                                           ^~

Do you have any ideas on how to produce a better error message?

@aochagavia
Copy link
Contributor Author

By the way... Who would ever try to pass type parameters to tuple indexing? Do we really want to have a special case for this? It could make the error message even more confusing in most cases.

The only case in which this could make sense is when a generic function is saved in a tuple. However, rust requires the type parameters to be specified when the function is stored in the tuple, not when it is called. For example:

fn something<T>(e: T) { ... }

// This works
let tup = (something::<int>, 42i);
tup.0();

// This doesn't
let tup = (something, 42i);
           ^ unable to infer enough type information about `_`; type annotations required
tup.0()::<int>;
       ^ found `::` expecting `;` or `}`

@ghost
Copy link

ghost commented Nov 23, 2014

@aochagavia Yeah, I think you're right! Feel free to push your current changes and I'm happy.

This breaks code like

```
let t = (42i, 42i);
... t.0::<int> ...;
```

Change this code to not contain an unused type parameter. For example:

```
let t = (42i, 42i);
... t.0 ...;
```

Closes #19096

[breaking-change]
@aochagavia
Copy link
Contributor Author

Done!

@aochagavia
Copy link
Contributor Author

I have added the error message ;)

@bors bors merged commit 40e1f8f into rust-lang:master Nov 23, 2014
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.

x.0::<T> is legal
3 participants