Fix issue: Global paths in `use` directives can begin with `super` or `self` #32225 #32403

vlastachu · 2016-03-21T15:48:08Z

This PR fixes #32225 by warning on use ::super::... and use ::self::... on resolve.

Current changes is the most minimal and ad-hoc.

rust-highfive · 2016-03-21T15:48:23Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

petrochenkov · 2016-03-21T16:21:27Z

This should probably be checked after macro expansion and not in parser (due to macros) + for all paths and not only for imports.
AFAIK, we don't have a dedicated "sanity check" post-expansion visitor in libsyntax, so lower_path/lower_view_path in librustc_front/lowering.rs may be the most appropriate place for this check.

vlastachu · 2016-03-21T19:02:08Z

@petrochenkov Okay, would try to remake in near future

jseyfried · 2016-03-22T08:16:12Z

@petrochenkov I believe this already checks macro expanded code since macro expanded code is parsed just like other code.

jseyfried · 2016-03-22T08:16:53Z

src/libsyntax/parse/parser.rs

@@ -1774,6 +1776,7 @@ impl<'a> Parser<'a> {
        })
    }

+


nit: There's no need for these added newlines

jseyfried · 2016-03-22T08:59:18Z

@vlastachu Thanks for the PR! I like this implementation.

Could you squash the existing commits and add a compile-fail test? For example,

use ::super::foo; //~ ERROR expected identifier, found keyword `super`

Also, the initial comment should be a summary of the PR (something like "This fixes #32225 by forbidding use ::super::... and use ::self::... during parsing").

jseyfried · 2016-03-22T09:12:07Z

@alexcrichton could you grep through the crates.io code for use ::self or use ::super to assess breakage?

jseyfried · 2016-03-22T09:12:31Z

r? @jseyfried

alexcrichton · 2016-03-22T16:50:04Z

A cursory grep yielded:

https://github.com/farseerfc/ydcv-rs/blob/55c267923994027edea64028d594cb315680d1bb/src/formatters.rs#L118
https://github.com/andelf/rust-algs4/blob/fb9ff77df4b1cd7ca1eff5749eff7b216a9db1f1/src/sorting/mod.rs#L53-L64
https://github.com/maidsafe/crust/blob/master/src/config_handler.rs (in version 0.8.0 of the crate, not the current version)

petrochenkov · 2016-03-22T19:08:36Z

@jseyfried

I believe this already checks macro expanded code since macro expanded code is parsed just like other code.

I meant something like

macro_rules! m {
    ($i: ident) => (::$i)
}

m!(self)

I'm not exactly sure if it's reparsed or not. (but m!(self) doesn't compile because self here isn't considered an ident for some reason). Anyway, there are procedural macros, they still can generate such paths.
This is also a semantic check and not syntactic, something like ::super::self::Self::super is a perfectly grammatical path and parsing can continue (this PR makes parsing stop after this error). IMO, such paths would be better rejected by resolve or a some other post-expansion sanity check pass, but it's not a blocker, at least this patch covers the common case.

vlastachu · 2016-03-22T22:06:22Z

@petrochenkov I can't reproduce successful compilation with macro. Looks like there is additional checking for keywords. Probably any way to cheat parser and give super to macro is existing. But I'm not found. I was trying to replace the ident type with something else.

::super::self::Self::super is a perfectly grammatical path

I don't get it. May be I don't understand this sentence right. Anyway global super is wrong. Probably there should be more general rule like "super, self or Self is appearing only at begin of global path" (the real rule may be more complicated) which should be checked before search of crates and modules. And such rule cancels necessity of my changes.
Are you about that?

vlastachu · 2016-03-22T22:11:42Z

@jseyfried

add a compile-fail test

done

Could you squash the existing commits

When all discussions will completed and last changes reviewed.

jseyfried · 2016-03-22T23:14:21Z

@petrochenkov

Anyway, there are procedural macros, they still can generate such paths.

Good point. I believe the path from your example is re-parsed but it doesn't really matter given procedural macros. Also, I agree that this should be a semantic rather than a syntactic error.

@vlastachu Reviewed all commits, the test looks good.

I think it would be best to have the check here, in the build_reduced_graph phase of resolve.

Since it won't add much complexity, I think we should do a warning cycle. This commit is an example of starting a warning cycle.

vlastachu · 2016-03-23T16:23:50Z

@jseyfried

I think it would be best to have the check here, in the build_reduced_graph phase of resolve.

Can you explain more about this? Am I remove all checking from parser and write similiar to pointed function? Then I should write it in single style with ResolutionError::SelfImportCanOnlyAppearOnceInTheList, ResolutionError::SelfImportOnlyInImportListWithNonEmptyPrefix.

At first I thought that it is necessary to make warnings and stopped on that I can't make similiar warning in parser session. If I should save current parser checking than could you help with add_lint for warning in parser context (probably there is should be any macro like span_warn!).

jseyfried · 2016-03-23T17:24:00Z

Yeah, there should be no checking in the parser, only in build_reduced_graph.rs.

I wouldn't write a helper function (I think this is what you mean by pointed function) but you can if you want. Also, I wouldn't bother with the ResolutionError enum.

Here is an example of a warning in resolve.

jseyfried · 2016-03-29T22:00:22Z

@vlastachu would you like to work on moving the check to build_reduced_graph.rs and making it a warning? If not, I can take it from here.

vlastachu · 2016-03-30T01:58:44Z

@jseyfried Yes, I would. Sory for silence. I was planning to do this at weekend. But for some reason I did not find the time. I will try to finish everything before the end of the week, or will notify you that I can not.

jseyfried · 2016-03-30T02:01:05Z

Thanks, no hurry!

vlastachu · 2016-03-30T23:35:06Z

@jseyfried here fullpath.global and here module_ident_path.global is always false.

Comments says that this flag works as expected.

jseyfried · 2016-03-30T23:47:44Z

Good point. I believe you can fix the flag by changing this line to

let is_global = self.eat(&token::ModSep);

and then replacing global: false with global: is_global (four times).

vlastachu · 2016-04-01T23:58:09Z

Checking may be made simpler if it necessary (like it was made before at parser)

if is_global && (name == SELF_KEYWORD_NAME || name == SUPER_KEYWORD_NAME)

Without unusual loop.

Now I see unresolved conflicts. Am I need to git rebase master ?

jseyfried · 2016-04-02T00:21:13Z

src/librustc/lint/builtin.rs

+declare_lint! {
+    pub SUPER_OR_SELF_IN_GLOBAL_MODNAME,
+    Warn,
+    "detects super or self keywords in begin of global path"


nit: "detects super or self keywords at the beginning of global paths"

jseyfried · 2016-04-02T00:32:10Z

@vlastachu Nice, looks good!

Checking may be made simpler if it necessary

I would prefer the simpler checking you described, at least until the warning becomes a hard error.

Now I see unresolved conflicts. Do I need to git rebase master?

Yeah. I would also squash all the commits into a single commit first, both for a cleaner history and for easier rebasing.

jseyfried · 2016-04-04T00:49:53Z

src/librustc/lint/builtin.rs

@@ -179,6 +179,12 @@ declare_lint! {
    "lints that have been renamed or removed"
 }

+declare_lint! {
+    pub SUPER_OR_SELF_IN_GLOBAL_MODNAME,


nit: rename to SUPER_OR_SELF_IN_GLOBAL_PATH

jseyfried · 2016-04-04T01:32:46Z

@vlastachu Excellent!

In your initial comment, could you change "forbidding" to "warning on" and "during parsing" to "in resolve" and also remove the part about the failed tests?

r=me with that edit and with the nit / tidy errors addressed.

jseyfried · 2016-04-04T06:22:55Z

src/librustc_resolve/build_reduced_graph.rs

@@ -137,6 +141,18 @@ impl<'b, 'tcx:'b> Resolver<'b, 'tcx> {
                    }
                };

+                // Checking for special identifiers in path
+                // prevent `self` or `supper` at beginning of global path


nit: .. or super at ..

Issue: rust-lang#32225

vlastachu · 2016-04-05T09:17:00Z

@jseyfried Complete.

jseyfried · 2016-04-05T09:26:26Z

@bors r+ 6c73134

Fix issue: Global paths in `use` directives can begin with `super` or `self` rust-lang#32225 This PR fixes rust-lang#32225 by warning on `use ::super::...` and `use ::self::...` on `resolve`. Current changes is the most minimal and ad-hoc.

Rollup of 11 pull requests - Successful merges: #32403, #32596, #32675, #32678, #32685, #32686, #32692, #32710, #32712, #32714, #32715 - Failed merges: #32488

rust-highfive assigned pnkfelix Mar 21, 2016

jseyfried reviewed Mar 22, 2016
View reviewed changes

src/libsyntax/parse/parser.rs

@@ -1774,6 +1776,7 @@ impl<'a> Parser<'a> {

})

}

Copy link

Contributor

jseyfried Mar 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: There's no need for these added newlines

vlastachu force-pushed the super_in_path branch from a5d463f to 654691a Compare April 1, 2016 23:46

jseyfried reviewed Apr 2, 2016
View reviewed changes

vlastachu force-pushed the super_in_path branch 3 times, most recently from 3a9b0af to 3982d07 Compare April 2, 2016 11:18

vlastachu force-pushed the super_in_path branch 8 times, most recently from 2fbedc8 to 16c3662 Compare April 2, 2016 22:57

jseyfried reviewed Apr 4, 2016
View reviewed changes

Fixes bug which accepting using super in use statemet.

6c73134

Issue: rust-lang#32225

vlastachu force-pushed the super_in_path branch from 16c3662 to 6c73134 Compare April 5, 2016 09:12

Manishearth mentioned this pull request Apr 5, 2016

Rollup of 11 pull requests #32743

Merged

bors added a commit that referenced this pull request Apr 5, 2016

Auto merge of #32743 - Manishearth:rollup, r=Manishearth

57e5d43

Rollup of 11 pull requests - Successful merges: #32403, #32596, #32675, #32678, #32685, #32686, #32692, #32710, #32712, #32714, #32715 - Failed merges: #32488

bors merged commit 6c73134 into rust-lang:master Apr 5, 2016

petrochenkov mentioned this pull request May 14, 2016

Mixed absolute-relative import is allowed in use delcaration #33636

Closed

petrochenkov mentioned this pull request Oct 1, 2016

Tracking issue for super_or_self_in_global_path compatibility lint #36888

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue: Global paths in `use` directives can begin with `super` or `self` #32225 #32403

Fix issue: Global paths in `use` directives can begin with `super` or `self` #32225 #32403

vlastachu commented Mar 21, 2016

rust-highfive commented Mar 21, 2016

petrochenkov commented Mar 21, 2016

vlastachu commented Mar 21, 2016

jseyfried commented Mar 22, 2016

jseyfried Mar 22, 2016

jseyfried commented Mar 22, 2016

jseyfried commented Mar 22, 2016

jseyfried commented Mar 22, 2016

alexcrichton commented Mar 22, 2016

petrochenkov commented Mar 22, 2016

vlastachu commented Mar 22, 2016

vlastachu commented Mar 22, 2016

jseyfried commented Mar 22, 2016

vlastachu commented Mar 23, 2016

jseyfried commented Mar 23, 2016

jseyfried commented Mar 29, 2016

vlastachu commented Mar 30, 2016

jseyfried commented Mar 30, 2016

vlastachu commented Mar 30, 2016

jseyfried commented Mar 30, 2016

vlastachu commented Apr 1, 2016

jseyfried Apr 2, 2016

jseyfried commented Apr 2, 2016

jseyfried Apr 4, 2016

jseyfried commented Apr 4, 2016

jseyfried Apr 4, 2016

vlastachu commented Apr 5, 2016

jseyfried commented Apr 5, 2016

Fix issue: Global paths in use directives can begin with super or self #32225 #32403

Fix issue: Global paths in use directives can begin with super or self #32225 #32403

Conversation

vlastachu commented Mar 21, 2016

rust-highfive commented Mar 21, 2016

petrochenkov commented Mar 21, 2016

vlastachu commented Mar 21, 2016

jseyfried commented Mar 22, 2016

jseyfried Mar 22, 2016

Choose a reason for hiding this comment

jseyfried commented Mar 22, 2016

jseyfried commented Mar 22, 2016

jseyfried commented Mar 22, 2016

alexcrichton commented Mar 22, 2016

petrochenkov commented Mar 22, 2016

vlastachu commented Mar 22, 2016

vlastachu commented Mar 22, 2016

jseyfried commented Mar 22, 2016

vlastachu commented Mar 23, 2016

jseyfried commented Mar 23, 2016

jseyfried commented Mar 29, 2016

vlastachu commented Mar 30, 2016

jseyfried commented Mar 30, 2016

vlastachu commented Mar 30, 2016

jseyfried commented Mar 30, 2016

vlastachu commented Apr 1, 2016

jseyfried Apr 2, 2016

Choose a reason for hiding this comment

jseyfried commented Apr 2, 2016

jseyfried Apr 4, 2016

Choose a reason for hiding this comment

jseyfried commented Apr 4, 2016

jseyfried Apr 4, 2016

Choose a reason for hiding this comment

vlastachu commented Apr 5, 2016

jseyfried commented Apr 5, 2016

Fix issue: Global paths in `use` directives can begin with `super` or `self` #32225 #32403

Fix issue: Global paths in `use` directives can begin with `super` or `self` #32225 #32403