Merge #4246

4246: Validate uses of self and super r=matklad a=djrenren

This change follows on the validation of the `crate` keyword in paths. It verifies the following things:

`super`:
 - May only be preceded by other `super` segments
 - If in a `UseItem` then all semantically preceding paths also consist only of `super`

`self`
 - May only be the start of a path


Just a note, a couple times while working on this I found myself really wanting a Visitor of some sort so that I could traverse descendants while skipping sub-trees that are unimportant. Iterators don't really work for this, so as you can see I reached for recursion. Considering paths are generally small a fancy debounced visitor probably isn't important but figured I'd say something in case we had something like this lying around and I wasn't using it.

Co-authored-by: John Renner <john@jrenner.net>

crates/ra_syntax/src/ast/generated/nodes.rs

@@ -1241,6 +1241,8 @@ pub struct PathSegment {
 impl PathSegment {
     pub fn coloncolon_token(&self) -> Option<SyntaxToken> { support::token(&self.syntax, T![::]) }
     pub fn crate_token(&self) -> Option<SyntaxToken> { support::token(&self.syntax, T![crate]) }
+    pub fn self_token(&self) -> Option<SyntaxToken> { support::token(&self.syntax, T![self]) }
+    pub fn super_token(&self) -> Option<SyntaxToken> { support::token(&self.syntax, T![super]) }
     pub fn l_angle_token(&self) -> Option<SyntaxToken> { support::token(&self.syntax, T![<]) }
     pub fn name_ref(&self) -> Option<NameRef> { support::child(&self.syntax) }
     pub fn type_arg_list(&self) -> Option<TypeArgList> { support::child(&self.syntax) }

crates/ra_syntax/src/validation.rs

@@ -96,7 +96,7 @@ pub(crate) fn validate(root: &SyntaxNode) -> Vec<SyntaxError> {
                 ast::RecordField(it) => validate_numeric_name(it.name_ref(), &mut errors),
                 ast::Visibility(it) => validate_visibility(it, &mut errors),
                 ast::RangeExpr(it) => validate_range_expr(it, &mut errors),
-                ast::PathSegment(it) => validate_crate_keyword_in_path_segment(it, &mut errors),
+                ast::PathSegment(it) => validate_path_keywords(it, &mut errors),
                 _ => (),
             }
         }
@@ -224,59 +224,82 @@ fn validate_range_expr(expr: ast::RangeExpr, errors: &mut Vec<SyntaxError>) {
     }
 }
 
-fn validate_crate_keyword_in_path_segment(
+fn validate_path_keywords(segment: ast::PathSegment, errors: &mut Vec<SyntaxError>) {
-    segment: ast::PathSegment,
+    use ast::PathSegmentKind;
-    errors: &mut Vec<SyntaxError>,
-) {
-    const ERR_MSG: &str = "The `crate` keyword is only allowed as the first segment of a path";
 
-    let crate_token = match segment.crate_token() {
+    let path = segment.parent_path();
-        None => return,
+    let is_path_start = segment.coloncolon_token().is_none() && path.qualifier().is_none();
-        Some(it) => it,
-    };
 
-    // Disallow both ::crate and foo::crate
+    if let Some(token) = segment.self_token() {
-    let mut path = segment.parent_path();
+        if !is_path_start {
-    if segment.coloncolon_token().is_some() || path.qualifier().is_some() {
+            errors.push(SyntaxError::new(
-        errors.push(SyntaxError::new(ERR_MSG, crate_token.text_range()));
+                "The `self` keyword is only allowed as the first segment of a path",
-        return;
+                token.text_range(),
-    }
+            ));
+        }
+    } else if let Some(token) = segment.crate_token() {
+        if !is_path_start || use_prefix(path).is_some() {
+            errors.push(SyntaxError::new(
+                "The `crate` keyword is only allowed as the first segment of a path",
+                token.text_range(),
+            ));
+        }
+    } else if let Some(token) = segment.super_token() {
+        if !all_supers(&path) {
+            errors.push(SyntaxError::new(
+                "The `super` keyword may only be preceded by other `super`s",
+                token.text_range(),
+            ));
+            return;
+        }
 
-    // For expressions and types, validation is complete, but we still have
+        let mut curr_path = path;
-    // to handle invalid UseItems like this:
+        while let Some(prefix) = use_prefix(curr_path) {
-    //
+            if !all_supers(&prefix) {
-    //      use foo:{crate::bar::baz};
+                errors.push(SyntaxError::new(
-    //
+                    "The `super` keyword may only be preceded by other `super`s",
-    // To handle this we must inspect the parent `UseItem`s and `UseTree`s
+                    token.text_range(),
-    // but right now we're looking deep inside the nested `Path` nodes because
+                ));
-    // `Path`s are left-associative:
+                return;
-    //
-    //   ((crate)::bar)::baz)
-    //       ^ current value of path
-    //
-    // So we need to climb to the top
-    while let Some(parent) = path.parent_path() {
-        path = parent;
-    }
-
-    // Now that we've found the whole path we need to see if there's a prefix
-    // somewhere in the UseTree hierarchy. This check is arbitrarily deep
-    // because rust allows arbitrary nesting like so:
-    //
-    // use {foo::{{{{crate::bar::baz}}}}};
-    for node in path.syntax().ancestors().skip(1) {
-        match_ast! {
-            match node {
-                ast::UseTree(it) => if let Some(tree_path) = it.path() {
-                    // Even a top-level path exists within a `UseTree` so we must explicitly
-                    // allow our path but disallow anything else
-                    if tree_path != path {
-                        errors.push(SyntaxError::new(ERR_MSG, crate_token.text_range()));
-                    }
-                },
-                ast::UseTreeList(_it) => continue,
-                _ => return,
             }
+            curr_path = prefix;
+        }
+    }
+
+    fn use_prefix(mut path: ast::Path) -> Option<ast::Path> {
+        for node in path.syntax().ancestors().skip(1) {
+            match_ast! {
+                match node {
+                    ast::UseTree(it) => if let Some(tree_path) = it.path() {
+                        // Even a top-level path exists within a `UseTree` so we must explicitly
+                        // allow our path but disallow anything else
+                        if tree_path != path {
+                            return Some(tree_path);
+                        }
+                    },
+                    ast::UseTreeList(_it) => continue,
+                    ast::Path(parent) => path = parent,
+                    _ => return None,
+                }
+            };
+        }
+        return None;
+    }
+
+    fn all_supers(path: &ast::Path) -> bool {
+        let segment = match path.segment() {
+            Some(it) => it,
+            None => return false,
         };
+
+        if segment.kind() != Some(PathSegmentKind::SuperKw) {
+            return false;
+        }
+
+        if let Some(ref subpath) = path.qualifier() {
+            return all_supers(subpath);
+        }
+
+        return true;
     }
 }

crates/ra_syntax/test_data/parser/err/0041_illegal_super_keyword_location.rast

@@ -0,0 +1,70 @@
+SOURCE_FILE@0..67
+  USE_ITEM@0..12
+    USE_KW@0..3 "use"
+    WHITESPACE@3..4 " "
+    USE_TREE@4..11
+      PATH@4..11
+        PATH_SEGMENT@4..11
+          COLON2@4..6 "::"
+          SUPER_KW@6..11 "super"
+    SEMICOLON@11..12 ";"
+  WHITESPACE@12..13 "\n"
+  USE_ITEM@13..26
+    USE_KW@13..16 "use"
+    WHITESPACE@16..17 " "
+    USE_TREE@17..25
+      PATH@17..25
+        PATH@17..18
+          PATH_SEGMENT@17..18
+            NAME_REF@17..18
+              IDENT@17..18 "a"
+        COLON2@18..20 "::"
+        PATH_SEGMENT@20..25
+          SUPER_KW@20..25 "super"
+    SEMICOLON@25..26 ";"
+  WHITESPACE@26..27 "\n"
+  USE_ITEM@27..47
+    USE_KW@27..30 "use"
+    WHITESPACE@30..31 " "
+    USE_TREE@31..46
+      PATH@31..46
+        PATH@31..39
+          PATH@31..36
+            PATH_SEGMENT@31..36
+              SUPER_KW@31..36 "super"
+          COLON2@36..38 "::"
+          PATH_SEGMENT@38..39
+            NAME_REF@38..39
+              IDENT@38..39 "a"
+        COLON2@39..41 "::"
+        PATH_SEGMENT@41..46
+          SUPER_KW@41..46 "super"
+    SEMICOLON@46..47 ";"
+  WHITESPACE@47..48 "\n"
+  USE_ITEM@48..66
+    USE_KW@48..51 "use"
+    WHITESPACE@51..52 " "
+    USE_TREE@52..65
+      PATH@52..53
+        PATH_SEGMENT@52..53
+          NAME_REF@52..53
+            IDENT@52..53 "a"
+      COLON2@53..55 "::"
+      USE_TREE_LIST@55..65
+        L_CURLY@55..56 "{"
+        USE_TREE@56..64
+          PATH@56..64
+            PATH@56..61
+              PATH_SEGMENT@56..61
+                SUPER_KW@56..61 "super"
+            COLON2@61..63 "::"
+            PATH_SEGMENT@63..64
+              NAME_REF@63..64
+                IDENT@63..64 "b"
+        R_CURLY@64..65 "}"
+    SEMICOLON@65..66 ";"
+  WHITESPACE@66..67 "\n"
+error 6..11: The `super` keyword may only be preceded by other `super`s
+error 20..25: The `super` keyword may only be preceded by other `super`s
+error 41..46: The `super` keyword may only be preceded by other `super`s
+error 56..61: The `super` keyword may only be preceded by other `super`s

crates/ra_syntax/test_data/parser/err/0041_illegal_super_keyword_location.rs

@@ -0,0 +1,4 @@
+use ::super;
+use a::super;
+use super::a::super;
+use a::{super::b};

crates/ra_syntax/test_data/parser/err/0042_illegal_self_keyword_location.rast

@@ -0,0 +1,27 @@
+SOURCE_FILE@0..25
+  USE_ITEM@0..11
+    USE_KW@0..3 "use"
+    WHITESPACE@3..4 " "
+    USE_TREE@4..10
+      PATH@4..10
+        PATH_SEGMENT@4..10
+          COLON2@4..6 "::"
+          SELF_KW@6..10 "self"
+    SEMICOLON@10..11 ";"
+  WHITESPACE@11..12 "\n"
+  USE_ITEM@12..24
+    USE_KW@12..15 "use"
+    WHITESPACE@15..16 " "
+    USE_TREE@16..23
+      PATH@16..23
+        PATH@16..17
+          PATH_SEGMENT@16..17
+            NAME_REF@16..17
+              IDENT@16..17 "a"
+        COLON2@17..19 "::"
+        PATH_SEGMENT@19..23
+          SELF_KW@19..23 "self"
+    SEMICOLON@23..24 ";"
+  WHITESPACE@24..25 "\n"
+error 6..10: The `self` keyword is only allowed as the first segment of a path
+error 19..23: The `self` keyword is only allowed as the first segment of a path

crates/ra_syntax/test_data/parser/err/0042_illegal_self_keyword_location.rs

@@ -0,0 +1,2 @@
+use ::self;
+use a::self;

crates/ra_syntax/test_data/parser/ok/0013_use_path_self_super.rast

@@ -1,4 +1,4 @@
-SOURCE_FILE@0..65
+SOURCE_FILE@0..38
   USE_ITEM@0..14
     USE_KW@0..3 "use"
     WHITESPACE@3..4 " "
@@ -31,27 +31,3 @@ SOURCE_FILE@0..65
             IDENT@33..36 "bar"
     SEMICOLON@36..37 ";"
   WHITESPACE@37..38 "\n"
-  USE_ITEM@38..64
-    USE_KW@38..41 "use"
-    WHITESPACE@41..42 " "
-    USE_TREE@42..63
-      PATH@42..63
-        PATH@42..58
-          PATH@42..51
-            PATH@42..48
-              PATH_SEGMENT@42..48
-                COLON2@42..44 "::"
-                SELF_KW@44..48 "self"
-            COLON2@48..50 "::"
-            PATH_SEGMENT@50..51
-              NAME_REF@50..51
-                IDENT@50..51 "a"
-          COLON2@51..53 "::"
-          PATH_SEGMENT@53..58
-            SUPER_KW@53..58 "super"
-        COLON2@58..60 "::"
-        PATH_SEGMENT@60..63
-          NAME_REF@60..63
-            IDENT@60..63 "bar"
-    SEMICOLON@63..64 ";"
-  WHITESPACE@64..65 "\n"

crates/ra_syntax/test_data/parser/ok/0013_use_path_self_super.rs

@@ -1,3 +1,2 @@
 use self::foo;
 use super::super::bar;
-use ::self::a::super::bar;

xtask/src/ast_src.rs

@@ -593,7 +593,7 @@ pub(crate) const AST_SRC: AstSrc = AstSrc {
             qualifier: Path,
         }
         struct PathSegment {
-            T![::], T![crate], T![<], NameRef, TypeArgList, ParamList, RetType, PathType, T![>]
+            T![::], T![crate], T![self], T![super], T![<], NameRef, TypeArgList, ParamList, RetType, PathType, T![>]
         }
         struct TypeArgList {
             T![::],