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

Suggest removing self import when not in a list #63741

Closed
varkor opened this issue Aug 20, 2019 · 8 comments · Fixed by #71863
Closed

Suggest removing self import when not in a list #63741

varkor opened this issue Aug 20, 2019 · 8 comments · Fixed by #71863
Assignees
Labels
A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@varkor
Copy link
Member

varkor commented Aug 20, 2019

use std::self; // error[E0429]: `self` imports are only allowed within a { } list

we should offer the suggestion to remove ::self, as this is likely what the user intended.

This issue has been assigned to @mibac138 via this comment.

@varkor varkor added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` labels Aug 20, 2019
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 20, 2019
@Centril
Copy link
Contributor

Centril commented Aug 20, 2019

@petrochenkov Would it simplify or complicate things if we simply allowed this instead of rejecting it?

@petrochenkov
Copy link
Contributor

~Simplify.

@Centril Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 20, 2019
@Centril
Copy link
Contributor

Centril commented Aug 20, 2019

Might be worth doing that in that case.

(Although use foo::bar::self; is less clean from a readability perspective but perhaps that should merely be a lint.)

@petrochenkov
Copy link
Contributor

It needs some amount design work though (not large).

The idea is to treat self and super in Rust paths as . and .. in filesystem paths.
Some questions arise in the process, e.g. if bar is an import (symlink), then whose parent bar::super refers too, or whether use foo::super [as name]; is allowed without the as name part and what name it introduces (use foo::self; needs to be allowed for compatibility and introduces foo).

Also cc #48067 (path concatenation in macros).

@nikomatsakis
Copy link
Contributor

Visited in compiler triage. I think this is medium priority, clearly, but I am guessing that @Centril intended the nomination for the lang team.

Wearing my lang team hat, I think I'd expect this to compile, but I don't have strong opinions about it.

@nikomatsakis
Copy link
Contributor

We discussed this in the lang team meeting last week. We decided that there were some reasons to tread carefully here: in particular, we didn't really want to admit the possibility of "non-canonical" paths like use foo::bar::self::super::bar. Moreover, there isn't a strong reason to accept this pattern -- it would definitely be linted against, and if you have some kind of tooling that wishes to emit self for simplicity, they can always emit things like use foo::bar::{self}.

So the conclusion was "probably better to just leave it alone for now" from a language perspective.

If we were to accept foo::self, we'd want some conditions:

  • always the last item in a path
  • do not accept super in that position, just self

@comex
Copy link
Contributor

comex commented Mar 23, 2020

In addition to a suggestion, the compiler could have better error recovery. Currently, the following produces errors on both the first and second line; it should only error on the first line:

use std::mem; // `self` imports are only allowed within a { } list
fn main() { mem::drop(42); } // use of undeclared type or module `mem`

@mibac138
Copy link
Contributor

mibac138 commented May 3, 2020

@rustbot claim

@rustbot rustbot self-assigned this May 3, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 20, 2020
Suggest fixes and add error recovery for `use foo::self`

Fixes rust-lang#63741.
I have implemented 2 suggestions on how to fix a `use foo::self` import, however I feel like showing them both might be too verbose.

Additionally, I have also implemented error recovery as [menitoned](rust-lang#63741 (comment)) by @comex.

I believe r? @estebank deals with diagnostics.
@bors bors closed this as completed in 14c4391 May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants