Repository: rust-analyzer/rust-analyzer
Pull request #11171 (2022-01-02 22:15:15)

fix: Fix tool module classification not working correctly
bors r+


Patch:
diff --git a/crates/hir/src/source_analyzer.rs b/crates/hir/src/source_analyzer.rs
index 2d779393f0..233c814809 100644
--- a/crates/hir/src/source_analyzer.rs
+++ b/crates/hir/src/source_analyzer.rs
@@ -290,9 +290,7 @@ impl SourceAnalyzer {
                 return Some(PathResolution::Def(ModuleDef::Variant(variant.into())));
             }
             prefer_value_ns = true;
-        }
-
-        if let Some(path_pat) = parent().and_then(ast::PathPat::cast) {
+        } else if let Some(path_pat) = parent().and_then(ast::PathPat::cast) {
             let pat_id = self.pat_id(&path_pat.into())?;
             if let Some(assoc) = self.infer.as_ref()?.assoc_resolutions_for_pat(pat_id) {
                 return Some(PathResolution::AssocItem(assoc.into()));
@@ -302,9 +300,7 @@ impl SourceAnalyzer {
             {
                 return Some(PathResolution::Def(ModuleDef::Variant(variant.into())));
             }
-        }
-
-        if let Some(rec_lit) = parent().and_then(ast::RecordExpr::cast) {
+        } else if let Some(rec_lit) = parent().and_then(ast::RecordExpr::cast) {
             let expr_id = self.expr_id(db, &rec_lit.into())?;
             if let Some(VariantId::EnumVariantId(variant)) =
                 self.infer.as_ref()?.variant_resolution_for_expr(expr_id)
@@ -331,32 +327,34 @@ impl SourceAnalyzer {
         // Case where path is a qualifier of a use tree, e.g. foo::bar::{Baz, Qux} where we are
         // trying to resolve foo::bar.
         if let Some(use_tree) = parent().and_then(ast::UseTree::cast) {
-            if let Some(qualifier) = use_tree.path() {
-                if path == &qualifier && use_tree.coloncolon_token().is_some() {
-                    return resolve_hir_path_qualifier(db, &self.resolver, &hir_path);
-                }
+            if use_tree.coloncolon_token().is_some() {
+                return resolve_hir_path_qualifier(db, &self.resolver, &hir_path);
             }
         }
 
         let is_path_of_attr = path
-            .top_path()
             .syntax()
             .ancestors()
-            .nth(2) // Path -> Meta -> Attr
-            .map_or(false, |it| ast::Attr::can_cast(it.kind()));
+            .map(|it| it.kind())
+            .take_while(|&kind| ast::Path::can_cast(kind) || ast::Meta::can_cast(kind))
+            .last()
+            .map_or(false, ast::Meta::can_cast);
 
         // Case where path is a qualifier of another path, e.g. foo::bar::Baz where we are
         // trying to resolve foo::bar.
-        if let Some(outer_path) = path.parent_path() {
-            if let Some(qualifier) = outer_path.qualifier() {
-                if path == &qualifier {
-                    return resolve_hir_path_qualifier(db, &self.resolver, &hir_path);
+        if path.parent_path().is_some() {
+            return match resolve_hir_path_qualifier(db, &self.resolver, &hir_path) {
+                None if is_path_of_attr => {
+                    path.first_segment().and_then(|it| it.name_ref()).and_then(|name_ref| {
+                        ToolModule::by_name(&name_ref.text()).map(PathResolution::ToolModule)
+                    })
                 }
-            }
+                res => res,
+            };
         } else if is_path_of_attr {
             // Case where we are resolving the final path segment of a path in an attribute
             // in this case we have to check for inert/builtin attributes and tools and prioritize
-            // resolution of attributes over other namesapces
+            // resolution of attributes over other namespaces
             let name_ref = path.as_single_name_ref();
             let builtin =
                 name_ref.as_ref().map(ast::NameRef::text).as_deref().and_then(BuiltinAttr::by_name);
@@ -365,27 +363,17 @@ impl SourceAnalyzer {
             }
             return match resolve_hir_path_as_macro(db, &self.resolver, &hir_path) {
                 res @ Some(m) if m.is_attr() => res.map(PathResolution::Macro),
-                _ => name_ref.and_then(|name_ref| {
+                // this labels any path that starts with a tool module as the tool itself, this is technically wrong
+                // but there is no benefit in differentiating these two cases for the time being
+                _ => path.first_segment().and_then(|it| it.name_ref()).and_then(|name_ref| {
                     ToolModule::by_name(&name_ref.text()).map(PathResolution::ToolModule)
                 }),
             };
         }
-
-        let res = if parent().map_or(false, |it| ast::Visibility::can_cast(it.kind())) {
+        if parent().map_or(false, |it| ast::Visibility::can_cast(it.kind())) {
             resolve_hir_path_qualifier(db, &self.resolver, &hir_path)
         } else {
             resolve_hir_path_(db, &self.resolver, &hir_path, prefer_value_ns)
-        };
-        match res {
-            Some(_) => res,
-            // this labels any path that starts with a tool module as the tool itself, this is technically wrong
-            // but there is no benefit in differentiating these two cases for the time being
-            None if is_path_of_attr => path
-                .first_segment()
-                .and_then(|seg| seg.name_ref())
-                .and_then(|name_ref| ToolModule::by_name(&name_ref.text()))
-                .map(PathResolution::ToolModule),
-            None => None,
         }
     }
 
diff --git a/crates/mbe/src/expander/matcher.rs b/crates/mbe/src/expander/matcher.rs
index 910724411d..bcda2381a4 100644
--- a/crates/mbe/src/expander/matcher.rs
+++ b/crates/mbe/src/expander/matcher.rs
@@ -571,18 +571,18 @@ fn match_loop(pattern: &MetaTemplate, src: &tt::Subtree) -> Match {
 
         if !error_items.is_empty() {
             error_recover_item = error_items.pop().map(|it| it.bindings);
-        } else if let [state, ..] = &*eof_items {
-            error_recover_item = Some(state.bindings.clone());
+        } else if !eof_items.is_empty() {
+            error_recover_item = Some(eof_items[0].bindings.clone());
         }
 
         // We need to do some post processing after the `match_loop_inner`.
         // If we reached the EOF, check that there is EXACTLY ONE possible matcher. Otherwise,
         // either the parse is ambiguous (which should never happen) or there is a syntax error.
         if src.peek_n(0).is_none() && stack.is_empty() {
-            if let [state] = &*eof_items {
+            if eof_items.len() == 1 {
                 // remove all errors, because it is the correct answer !
                 res = Match::default();
-                res.bindings = bindings_builder.build(&state.bindings);
+                res.bindings = bindings_builder.build(&eof_items[0].bindings);
             } else {
                 // Error recovery
                 if let Some(item) = error_recover_item {
@@ -598,10 +598,10 @@ fn match_loop(pattern: &MetaTemplate, src: &tt::Subtree) -> Match {
         //
         // Another possibility is that we need to call out to parse some rust nonterminal
         // (black-box) parser. However, if there is not EXACTLY ONE of these, something is wrong.
-        let has_leftover_tokens = (bb_items.is_empty() && next_items.is_empty())
-            || !(bb_items.is_empty() || next_items.is_empty())
-            || bb_items.len() > 1;
-        if has_leftover_tokens {
+        if (bb_items.is_empty() && next_items.is_empty())
+            || (!bb_items.is_empty() && !next_items.is_empty())
+            || bb_items.len() > 1
+        {
             res.unmatched_tts += src.len();
             while let Some(it) = stack.pop() {
                 src = it;
@@ -624,11 +624,7 @@ fn match_loop(pattern: &MetaTemplate, src: &tt::Subtree) -> Match {
                     stack.push(src.clone());
                     src = TtIter::new(subtree);
                 }
-                None => {
-                    if let Some(iter) = stack.pop() {
-                        src = iter;
-                    }
-                }
+                None if !stack.is_empty() => src = stack.pop().unwrap(),
                 _ => (),
             }
         }
@@ -666,23 +662,29 @@ fn match_loop(pattern: &MetaTemplate, src: &tt::Subtree) -> Match {
 fn match_leaf(lhs: &tt::Leaf, src: &mut TtIter) -> Result<(), ExpandError> {
     let rhs = match src.expect_leaf() {
         Ok(l) => l,
-        Err(()) => return Err(err!("expected leaf: `{}`", lhs)),
+        Err(()) => {
+            return Err(err!("expected leaf: `{}`", lhs));
+        }
     };
     match (lhs, rhs) {
         (
             tt::Leaf::Punct(tt::Punct { char: lhs, .. }),
             tt::Leaf::Punct(tt::Punct { char: rhs, .. }),
-        ) if lhs == rhs => Ok(()),
+        ) if lhs == rhs => (),
         (
             tt::Leaf::Ident(tt::Ident { text: lhs, .. }),
             tt::Leaf::Ident(tt::Ident { text: rhs, .. }),
-        ) if lhs == rhs => Ok(()),
+        ) if lhs == rhs => (),
         (
             tt::Leaf::Literal(tt::Literal { text: lhs, .. }),
             tt::Leaf::Literal(tt::Literal { text: rhs, .. }),
-        ) if lhs == rhs => Ok(()),
-        _ => Err(ExpandError::UnexpectedToken),
+        ) if lhs == rhs => (),
+        _ => {
+            return Err(ExpandError::UnexpectedToken);
+        }
     }
+
+    Ok(())
 }
 
 fn match_meta_var(kind: &str, input: &mut TtIter) -> ExpandResult<Option<Fragment>> {
diff --git a/crates/mbe/src/expander/transcriber.rs b/crates/mbe/src/expander/transcriber.rs
index b5ae5b91bb..98b37dd122 100644
--- a/crates/mbe/src/expander/transcriber.rs
+++ b/crates/mbe/src/expander/transcriber.rs
@@ -4,10 +4,11 @@
 use syntax::SmolStr;
 use tt::{Delimiter, Subtree};
 
+use super::ExpandResult;
 use crate::{
     expander::{Binding, Bindings, Fragment},
     parser::{Op, RepeatKind, Separator},
-    ExpandError, ExpandResult, MetaTemplate,
+    ExpandError, MetaTemplate,
 };
 
 impl Bindings {
@@ -16,36 +17,36 @@ impl Bindings {
     }
 
     fn get(&self, name: &str, nesting: &mut [NestingState]) -> Result<&Fragment, ExpandError> {
-        macro_rules! binding_err {
-            ($($arg:tt)*) => { ExpandError::BindingError(format!($($arg)*)) };
-        }
-
-        let mut b: &Binding = self
-            .inner
-            .get(name)
-            .ok_or_else(|| binding_err!("could not find binding `{}`", name))?;
+        let mut b: &Binding = self.inner.get(name).ok_or_else(|| {
+            ExpandError::BindingError(format!("could not find binding `{}`", name))
+        })?;
         for nesting_state in nesting.iter_mut() {
             nesting_state.hit = true;
             b = match b {
                 Binding::Fragment(_) => break,
                 Binding::Nested(bs) => bs.get(nesting_state.idx).ok_or_else(|| {
                     nesting_state.at_end = true;
-                    binding_err!("could not find nested binding `{}`", name)
+                    ExpandError::BindingError(format!("could not find nested binding `{}`", name))
                 })?,
                 Binding::Empty => {
                     nesting_state.at_end = true;
-                    return Err(binding_err!("could not find empty binding `{}`", name));
+                    return Err(ExpandError::BindingError(format!(
+                        "could not find empty binding `{}`",
+                        name
+                    )));
                 }
             };
         }
         match b {
             Binding::Fragment(it) => Ok(it),
-            Binding::Nested(_) => {
-                Err(binding_err!("expected simple binding, found nested binding `{}`", name))
-            }
-            Binding::Empty => {
-                Err(binding_err!("expected simple binding, found empty binding `{}`", name))
-            }
+            Binding::Nested(_) => Err(ExpandError::BindingError(format!(
+                "expected simple binding, found nested binding `{}`",
+                name
+            ))),
+            Binding::Empty => Err(ExpandError::BindingError(format!(
+                "expected simple binding, found empty binding `{}`",
+                name
+            ))),
         }
     }
 }
@@ -108,7 +109,7 @@ fn expand_subtree(
         }
     }
     // drain the elements added in this instance of expand_subtree
-    let tts = arena.drain(start_elements..).collect();
+    let tts = arena.drain(start_elements..arena.len()).collect();
     ExpandResult { value: tt::Subtree { delimiter, token_trees: tts }, err }
 }
 
@@ -192,22 +193,23 @@ fn expand_repeat(
         push_subtree(&mut buf, t);
 
         if let Some(sep) = separator {
-            has_seps = match sep {
+            match sep {
                 Separator::Ident(ident) => {
+                    has_seps = 1;
                     buf.push(tt::Leaf::from(ident.clone()).into());
-                    1
                 }
                 Separator::Literal(lit) => {
+                    has_seps = 1;
                     buf.push(tt::Leaf::from(lit.clone()).into());
-                    1
                 }
+
                 Separator::Puncts(puncts) => {
-                    for &punct in puncts {
-                        buf.push(tt::Leaf::from(punct).into());
+                    has_seps = puncts.len();
+                    for punct in puncts {
+                        buf.push(tt::Leaf::from(*punct).into());
                     }
-                    puncts.len()
                 }
-            };
+            }
         }
 
         if RepeatKind::ZeroOrOne == kind {
diff --git a/crates/mbe/src/parser.rs b/crates/mbe/src/parser.rs
index ef29380557..7543054494 100644
--- a/crates/mbe/src/parser.rs
+++ b/crates/mbe/src/parser.rs
@@ -41,7 +41,7 @@ impl MetaTemplate {
         let mut res = Vec::new();
         while let Some(first) = src.next() {
             let op = next_op(first, &mut src, mode)?;
-            res.push(op);
+            res.push(op)
         }
 
         Ok(MetaTemplate(res))
@@ -110,6 +110,12 @@ macro_rules! err {
     };
 }
 
+macro_rules! bail {
+    ($($tt:tt)*) => {
+        return Err(err!($($tt)*))
+    };
+}
+
 fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Result<Op, ParseError> {
     let res = match first {
         tt::TokenTree::Leaf(leaf @ tt::Leaf::Punct(tt::Punct { char: '$', .. })) => {
@@ -125,24 +131,26 @@ fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Resul
                     Op::Repeat { tokens, separator, kind }
                 }
                 tt::TokenTree::Leaf(leaf) => match leaf {
+                    tt::Leaf::Punct(_) => return Err(ParseError::Expected("ident".to_string())),
                     tt::Leaf::Ident(ident) if ident.text == "crate" => {
                         // We simply produce identifier `$crate` here. And it will be resolved when lowering ast to Path.
                         Op::Leaf(tt::Leaf::from(tt::Ident { text: "$crate".into(), id: ident.id }))
                     }
                     tt::Leaf::Ident(ident) => {
-                        let kind = eat_fragment_kind(src, mode)?;
                         let name = ident.text.clone();
-                        let id = ident.id;
-                        Op::Var { name, kind, id }
-                    }
-                    tt::Leaf::Literal(lit) if is_boolean_literal(lit) => {
                         let kind = eat_fragment_kind(src, mode)?;
-                        let name = lit.text.clone();
-                        let id = lit.id;
+                        let id = ident.id;
                         Op::Var { name, kind, id }
                     }
-                    tt::Leaf::Punct(_) | tt::Leaf::Literal(_) => {
-                        return Err(ParseError::Expected("ident".to_string()))
+                    tt::Leaf::Literal(lit) => {
+                        if is_boolean_literal(lit) {
+                            let name = lit.text.clone();
+                            let kind = eat_fragment_kind(src, mode)?;
+                            let id = lit.id;
+                            Op::Var { name, kind, id }
+                        } else {
+                            bail!("bad var 2");
+                        }
                     }
                 },
             }
@@ -158,8 +166,8 @@ fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Resul
 
 fn eat_fragment_kind(src: &mut TtIter<'_>, mode: Mode) -> Result<Option<SmolStr>, ParseError> {
     if let Mode::Pattern = mode {
-        src.expect_char(':').map_err(|()| err!("missing fragment specifier"))?;
-        let ident = src.expect_ident().map_err(|()| err!("missing fragment specifier"))?;
+        src.expect_char(':').map_err(|()| err!("bad fragment specifier 1"))?;
+        let ident = src.expect_ident().map_err(|()| err!("bad fragment specifier 1"))?;
         return Ok(Some(ident.text.clone()));
     };
     Ok(None)
@@ -191,15 +199,21 @@ fn parse_repeat(src: &mut TtIter) -> Result<(Option<Separator>, RepeatKind), Par
                     '*' => RepeatKind::ZeroOrMore,
                     '+' => RepeatKind::OneOrMore,
                     '?' => RepeatKind::ZeroOrOne,
-                    _ => match &mut separator {
-                        Separator::Puncts(puncts) if puncts.len() != 3 => {
-                            puncts.push(*punct);
-                            continue;
+                    _ => {
+                        match &mut separator {
+                            Separator::Puncts(puncts) => {
+                                if puncts.len() == 3 {
+                                    return Err(ParseError::InvalidRepeat);
+                                }
+                                puncts.push(*punct)
+                            }
+                            _ => return Err(ParseError::InvalidRepeat),
                         }
-                        _ => return Err(ParseError::InvalidRepeat),
-                    },
+                        continue;
+                    }
                 };
-                return Ok((has_sep.then(|| separator), repeat_kind));
+                let separator = if has_sep { Some(separator) } else { None };
+                return Ok((separator, repeat_kind));
             }
         }
     }


Test patch:
diff --git a/crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs b/crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs
index 2de10ddbdf..dd5effa368 100644
--- a/crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs
+++ b/crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs
@@ -69,11 +69,11 @@ macro_rules! e3 { ($(i:ident)_) => () }
 /* error: invalid macro definition: invalid repeat */
 
 macro_rules! f1 { ($i) => ($i) }
-/* error: invalid macro definition: missing fragment specifier */
+/* error: invalid macro definition: bad fragment specifier 1 */
 macro_rules! f2 { ($i:) => ($i) }
-/* error: invalid macro definition: missing fragment specifier */
+/* error: invalid macro definition: bad fragment specifier 1 */
 macro_rules! f3 { ($i:_) => () }
-/* error: invalid macro definition: missing fragment specifier */
+/* error: invalid macro definition: bad fragment specifier 1 */
 "#]],
     )
 }
diff --git a/crates/ide/src/syntax_highlighting/test_data/highlighting.html b/crates/ide/src/syntax_highlighting/test_data/highlighting.html
index 1ae07a125f..3ec6b6a77d 100644
--- a/crates/ide/src/syntax_highlighting/test_data/highlighting.html
+++ b/crates/ide/src/syntax_highlighting/test_data/highlighting.html
@@ -44,6 +44,7 @@ pre                 { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd
 <span class="keyword">mod</span> <span class="module declaration">inner</span> <span class="brace">{</span><span class="brace">}</span>
 
 <span class="attribute_bracket attribute">#</span><span class="attribute_bracket attribute">[</span><span class="builtin_attr attribute library">allow</span><span class="parenthesis attribute">(</span><span class="parenthesis attribute">)</span><span class="attribute_bracket attribute">]</span>
+<span class="attribute_bracket attribute">#</span><span class="attribute_bracket attribute">[</span><span class="tool_module attribute library">rustfmt</span><span class="operator attribute">::</span><span class="tool_module attribute library">skip</span><span class="attribute_bracket attribute">]</span>
 <span class="attribute_bracket attribute">#</span><span class="attribute_bracket attribute">[</span><span class="module attribute crate_root library">proc_macros</span><span class="operator attribute">::</span><span class="attribute attribute library">identity</span><span class="attribute_bracket attribute">]</span>
 <span class="keyword">pub</span> <span class="keyword">mod</span> <span class="module declaration public">ops</span> <span class="brace">{</span>
     <span class="attribute_bracket attribute">#</span><span class="attribute_bracket attribute">[</span><span class="builtin_attr attribute library">lang</span> <span class="operator attribute">=</span> <span class="string_literal attribute">"fn_once"</span><span class="attribute_bracket attribute">]</span>
diff --git a/crates/ide/src/syntax_highlighting/tests.rs b/crates/ide/src/syntax_highlighting/tests.rs
index c5834e1942..4e62edc7c1 100644
--- a/crates/ide/src/syntax_highlighting/tests.rs
+++ b/crates/ide/src/syntax_highlighting/tests.rs
@@ -17,6 +17,7 @@ use inner::{self as inner_mod};
 mod inner {}
 
 #[allow()]
+#[rustfmt::skip]
 #[proc_macros::identity]
 pub mod ops {
     #[lang = "fn_once"]