Attach outer attributes to the elements they annotate #245
Conversation
|
I have improved the description of the issue in #244 to make the problem clearer. Let me know if the issue or this PR needs any further improvement. |
|
Fishing for reviews for this one… perhaps @amaanq has thoughts on this PR? |
wetneb
force-pushed
the
244-attribute-parsing
branch
from
5e6b8aa to
9a51d0a
Compare
|
Hello @maxbrunsfeld, I am reaching out to you as you seem to be the only person who merged PRs in this repository over the past months. I wonder if I can hope to get this PR reviewed at some point, or if there is anything I could do to help with the PR backlog on this repository. Intuitively, I would suggest empowering more contributors to review PRs. For instance, @m4rch3n1ng has been making a lot of (in my opinion) very valuable contributions. Perhaps it would be worth reaching out to them to see if they would be interested in helping out with PR reviews? Thanks for your amazing work! |
There was a problem hiding this comment.
i know that i don't have any rights for this, but since i have been asked i figured i'd just give my, well, solicited review here.
this pr introduces a pretty major regression and there is now a lot of valid code that rustc accepts, tree-sitter-rust on the master branch accepts, but this pr doesn't. i have compiled a few examples here:
fn main() {
#[cfg(unix)]
{
println!("unix");
}
}#[allow(unused_macros)]
macro_rules! cfg {
() => {};
}
fn main() {}fn main() {
#[allow(unused_parens)]
match 0 {
(pat) => assert_eq!(pat, 0),
}
}additionally, while rustc doesn't want to compile the following code, it is happy to give me an ast-tree and will compile on nightly with a feature flag:
fn test(_: (u8, u8, u8, u8, u8)) {}
fn main() {
#[rustfmt::skip]
test(
(0, 1, 2, 3,
4, )
);
}additionally here is a diff of all rustc tests, that are valid and compile with rustc, but have regressed with this pr to no longer work with tree-sitter. this, in total, comes out to 87 tests:
diff
--- .tmp/list.txt.now 2025-03-19 15:13:08.220936488 +0100
+++ .tmp/list.txt.244-attribute-parsing 2025-03-19 15:11:21.650635556 +0100
@@ -1,4 +1,7 @@
+tests/assembly/closure-inherit-target-feature.rs
tests/codegen/async-closure-debug.rs
+tests/codegen/coroutine-debug-msvc.rs
+tests/codegen/coroutine-debug.rs
tests/codegen/dst-offset.rs
tests/codegen/function-arguments.rs
tests/codegen/sanitizer/cfi/emit-type-metadata-id-itanium-cxx-abi-user-defined-types.rs
@@ -12,25 +15,39 @@
tests/crashes/104685.rs
tests/crashes/116979.rs
tests/crashes/123887.rs
+tests/debuginfo/auxiliary/macro-stepping.rs
+tests/debuginfo/collapse-debuginfo-external-attr.rs
+tests/debuginfo/collapse-debuginfo-external-flag-overriden-by-attr.rs
+tests/debuginfo/collapse-debuginfo-in-non-collapse-macro.rs
+tests/debuginfo/collapse-debuginfo-static-external.rs
+tests/debuginfo/collapse-debuginfo-static.rs
+tests/debuginfo/collapse-debuginfo-with-attr-flag.rs
+tests/debuginfo/collapse-debuginfo-with-attr.rs
tests/debuginfo/coroutine-closure.rs
tests/debuginfo/coroutine-locals.rs
tests/debuginfo/coroutine-objects.rs
tests/debuginfo/function-names.rs
tests/debuginfo/issue-57822.rs
+tests/debuginfo/skip_second_statement_collapse.rs
tests/debuginfo/type-names.rs
+tests/incremental/macro_export.rs
tests/mir-opt/async_closure_shims.rs
tests/mir-opt/building/match/never_patterns.rs
tests/mir-opt/coroutine_drop_cleanup.rs
tests/mir-opt/coroutine_storage_dead_unwind.rs
tests/mir-opt/coroutine_tiny.rs
+tests/mir-opt/inline/inline_coroutine.rs
tests/mir-opt/tail_call_drops.rs
tests/pretty/delegation.rs
tests/pretty/macro.rs
tests/pretty/stmt_expr_attributes.rs
tests/pretty/yeet-expr.rs
+tests/run-make/lto-linkage-used-attr/lib.rs
tests/rustdoc-json/traits/trait_alias.rs
tests/rustdoc/macro-in-async-block.rs
tests/rustdoc/macro-in-closure.rs
+tests/ui/abi/simd-abi-checks-avx.rs
+tests/ui/anon-params/auxiliary/anon-params-edition-hygiene.rs
tests/ui/associated-consts/assoc-const-eq-bound-var-in-ty.rs
tests/ui/associated-consts/assoc-const-eq-supertraits.rs
tests/ui/associated-consts/assoc-const-eq-ty-alias-noninteracting.rs
@@ -67,15 +84,20 @@
tests/ui/async-await/for-await.rs
tests/ui/async-await/issues/issue-65419/issue-65419-coroutine-resume-after-completion.rs
tests/ui/async-await/no-params-non-move-async-closure.rs
+tests/ui/async-await/non-trivial-drop.rs
tests/ui/async-await/pin-ergonomics/sugar.rs
tests/ui/async-await/return-type-notation/issue-110963-late.rs
tests/ui/async-await/return-type-notation/normalizing-self-auto-trait-issue-109924.rs
tests/ui/async-await/return-type-notation/rtn-implied-in-supertrait.rs
tests/ui/async-await/return-type-notation/super-method-bound.rs
tests/ui/async-await/return-type-notation/supertrait-bound.rs
+tests/ui/attributes/invalid_macro_export_argument.rs
tests/ui/attributes/issue-115264-pat-field.rs
+tests/ui/attributes/tool_attributes.rs
+tests/ui/binding/pat-tuple-7.rs
tests/ui/borrowck/alias-liveness/higher-ranked.rs
tests/ui/borrowck/alias-liveness/rtn-static.rs
+tests/ui/check-cfg/allow-macro-cfg.rs
tests/ui/closures/binder/async-closure-with-binder.rs
tests/ui/closures/binder/late-bound-in-body.rs
tests/ui/closures/binder/nested-closures-regions.rs
@@ -89,40 +111,66 @@
tests/ui/const-generics/generic_const_exprs/drop_impl.rs
tests/ui/const-generics/generic_const_exprs/issue-90847.rs
tests/ui/const-generics/issues/issue-88119.rs
+tests/ui/const-generics/min_const_generics/macro.rs
tests/ui/consts/const-eval/issue-55541.rs
tests/ui/consts/const-labeled-break.rs
+tests/ui/consts/issue-65348.rs
+tests/ui/consts/promotion.rs
+tests/ui/consts/references.rs
tests/ui/consts/trait_specialization.rs
tests/ui/coroutine/addassign-yield.rs
tests/ui/coroutine/async-gen-deduce-yield.rs
tests/ui/coroutine/async-gen-yield-ty-is-unit.rs
tests/ui/coroutine/async_gen_fn_iter.rs
tests/ui/coroutine/borrow-in-tail-expr.rs
+tests/ui/coroutine/clone-rpit.rs
tests/ui/coroutine/conditional-drop.rs
tests/ui/coroutine/coroutine-resume-after-panic.rs
tests/ui/coroutine/derived-drop-parent-expr.rs
+tests/ui/coroutine/discriminant.rs
tests/ui/coroutine/drop-and-replace.rs
tests/ui/coroutine/drop-control-flow.rs
tests/ui/coroutine/drop-env.rs
tests/ui/coroutine/drop-track-addassign-yield.rs
tests/ui/coroutine/drop-tracking-yielding-in-match-guards.rs
tests/ui/coroutine/issue-110929-coroutine-conflict-error-ice.rs
+tests/ui/coroutine/issue-44197.rs
+tests/ui/coroutine/issue-52304.rs
+tests/ui/coroutine/issue-52398.rs
+tests/ui/coroutine/issue-57084.rs
+tests/ui/coroutine/issue-58888.rs
tests/ui/coroutine/issue-61442-stmt-expr-with-drop.rs
+tests/ui/coroutine/issue-69017.rs
+tests/ui/coroutine/issue-69039.rs
+tests/ui/coroutine/issue-87142.rs
+tests/ui/coroutine/iterator-count.rs
tests/ui/coroutine/live-upvar-across-yield.rs
+tests/ui/coroutine/match-bindings.rs
tests/ui/coroutine/nested_coroutine.rs
tests/ui/coroutine/niche-in-coroutine.rs
+tests/ui/coroutine/other-attribute-on-gen.rs
tests/ui/coroutine/overlap-locals.rs
tests/ui/coroutine/panic-drops-resume.rs
tests/ui/coroutine/panic-drops.rs
tests/ui/coroutine/panic-safe.rs
+tests/ui/coroutine/reborrow-mut-upvar.rs
tests/ui/coroutine/reinit-in-match-guard.rs
tests/ui/coroutine/resume-after-return.rs
tests/ui/coroutine/resume-arg-size.rs
tests/ui/coroutine/resume-live-across-yield.rs
tests/ui/coroutine/return-types-diverge.rs
+tests/ui/coroutine/size-moved-locals.rs
tests/ui/coroutine/smoke-resume-args.rs
tests/ui/coroutine/static-coroutine.rs
tests/ui/coroutine/static-mut-reference-across-yield.rs
+tests/ui/coroutine/static-reference-across-yield.rs
+tests/ui/coroutine/too-live-local-in-immovable-gen.rs
tests/ui/coroutine/uninhabited-field.rs
+tests/ui/coroutine/yield-in-args-rev.rs
+tests/ui/coroutine/yield-in-initializer.rs
+tests/ui/coroutine/yield-subtype.rs
+tests/ui/cross/cross-crate-macro-backtrace/auxiliary/extern_macro_crate.rs
+tests/ui/debuginfo/debuginfo-inline-callsite-location-macro-1.rs
tests/ui/definition-reachable/private-non-types.rs
tests/ui/definition-reachable/private-types.rs
tests/ui/delegation/body-identity-glob.rs
@@ -150,6 +198,7 @@
tests/ui/derives/derive-hygiene.rs
tests/ui/destructuring-assignment/struct_destructure.rs
tests/ui/drop/dynamic-drop.rs
+tests/ui/drop/lint-if-let-rescope.rs
tests/ui/dyn-compatibility/assoc_const_bounds.rs
tests/ui/dyn-compatibility/assoc_const_bounds_sized.rs
tests/ui/dyn-star/box.rs
@@ -173,8 +222,11 @@
tests/ui/explicit-tail-calls/ctfe-collatz-multi-rec.rs
tests/ui/explicit-tail-calls/return-lifetime-sub.rs
tests/ui/explicit-tail-calls/two-phase.rs
+tests/ui/expr/if/attrs/builtin-if-attr.rs
+tests/ui/feature-gates/feature-gate-asm_experimental_arch.rs
tests/ui/feature-gates/feature-gate-trivial_bounds-lint.rs
tests/ui/for-loop-while/loop-break-value.rs
+tests/ui/for-loop-while/loop-label-shadowing.rs
tests/ui/generic-associated-types/collections.rs
tests/ui/generic-const-items/assoc-const-AnonConst-ice-108220.rs
tests/ui/generic-const-items/associated-const-equality.rs
@@ -185,37 +237,62 @@
tests/ui/half-open-range-patterns/range_pat_interactions0.rs
tests/ui/higher-ranked/trait-bounds/issue-95230.rs
tests/ui/hygiene/assoc_ty_bindings.rs
+tests/ui/hygiene/eager-from-opaque-2.rs
tests/ui/hygiene/generic_params.rs
tests/ui/hygiene/issue-44128.rs
tests/ui/hygiene/items.rs
tests/ui/hygiene/lexical.rs
+tests/ui/hygiene/macro-metavars-transparent.rs
tests/ui/hygiene/specialization.rs
tests/ui/hygiene/stdlib-prelude-from-opaque-late.rs
tests/ui/hygiene/trait_items-2.rs
tests/ui/hygiene/traits-in-scope.rs
tests/ui/impl-trait/equality-rpass.rs
+tests/ui/impl-trait/lifetimes.rs
+tests/ui/impl-trait/recursive-coroutine-boxed.rs
tests/ui/imports/local-modularized-tricky-pass-2.rs
tests/ui/inline-const/const-match-pat-inference.rs
tests/ui/inline-const/const-match-pat-range.rs
tests/ui/inline-const/pat-unsafe.rs
+tests/ui/issues/issue-30530.rs
tests/ui/issues/issue-36116.rs
tests/ui/issues/issue-37051.rs
tests/ui/issues/issue-37733.rs
tests/ui/issues/issue-43692.rs
tests/ui/issues/issue-49632.rs
tests/ui/issues/issue-61475.rs
+tests/ui/iterators/into-iter-on-arrays-lint.rs
+tests/ui/iterators/into-iter-on-boxed-slices-lint.rs
tests/ui/lifetimes/elided-lifetime-in-param-pat.rs
+tests/ui/lint/auxiliary/lint_stability.rs
tests/ui/lint/dead-code/anon-const-in-pat.rs
+tests/ui/lint/expansion-time.rs
+tests/ui/lint/inert-attr-macro.rs
tests/ui/lint/invalid-nan-comparison-suggestion.rs
+tests/ui/lint/issue-49588-non-shorthand-field-patterns-in-pattern-macro.rs
+tests/ui/lint/lint-level-macro-def-mod.rs
+tests/ui/lint/lint-level-macro-def.rs
+tests/ui/lint/lints-on-stmt-not-overridden-130142.rs
+tests/ui/lint/rfc-2383-lint-reason/expect_lint_from_macro.rs
+tests/ui/lint/rfc-2383-lint-reason/force_warn_expected_lints_fulfilled.rs
+tests/ui/lint/rfc-2383-lint-reason/force_warn_expected_lints_unfulfilled.rs
+tests/ui/lint/rfc-2383-lint-reason/fulfilled_expectation_early_lints.rs
tests/ui/lint/semicolon-in-expressions-from-macros/semicolon-in-expressions-from-macros.rs
tests/ui/lint/unused/trait-alias-supertrait.rs
tests/ui/liveness/liveness-upvars.rs
+tests/ui/macros/auxiliary/issue-19163.rs
+tests/ui/macros/auxiliary/macro_with_super_1.rs
+tests/ui/macros/auxiliary/or-pattern.rs
tests/ui/macros/edition-macro-pats.rs
tests/ui/macros/expr_2021.rs
+tests/ui/macros/issue-33185.rs
tests/ui/macros/issue-44127.rs
+tests/ui/macros/issue-52169.rs
tests/ui/macros/issue-63102.rs
tests/ui/macros/issue-77475.rs
+tests/ui/macros/macro-crate-use.rs
tests/ui/macros/macro-in-fn.rs
+tests/ui/macros/macro-literal.rs
tests/ui/macros/macro-metavar-expr-concat/allowed-operations.rs
tests/ui/macros/macro-metavar-expr-concat/unicode-expansion.rs
tests/ui/macros/macro-nested_expr.rs
@@ -229,10 +306,14 @@
tests/ui/match/postfix-match/pf-match-chain.rs
tests/ui/mir/issue-68841.rs
tests/ui/nll/assign-while-to-immutable.rs
+tests/ui/nll/coroutine-distinct-lifetime.rs
tests/ui/nll/empty-type-predicate.rs
+tests/ui/nll/extra-unused-mut.rs
tests/ui/nll/issue-112604-closure-output-normalize.rs
+tests/ui/nll/issue-48623-coroutine.rs
tests/ui/packed/packed-struct-drop-aligned.rs
tests/ui/parser/impls-nested-within-fns-semantic-1.rs
+tests/ui/parser/issues/issue-65846-rollback-gating-failing-matcher.rs
tests/ui/parser/issues/issue-88583-union-as-ident.rs
tests/ui/parser/keyword-union-as-identifier.rs
tests/ui/parser/parser-unicode-whitespace.rs
@@ -245,7 +326,10 @@
tests/ui/rfcs/rfc-0000-never_patterns/diverges.rs
tests/ui/rfcs/rfc-0000-never_patterns/use-bindings.rs
tests/ui/rfcs/rfc-2008-non-exhaustive/structs_same_crate.rs
+tests/ui/rfcs/rfc-2091-track-caller/macro-declaration.rs
tests/ui/rfcs/rfc-2091-track-caller/tracked-closure.rs
+tests/ui/rfcs/rfc-2294-if-let-guard/run-pass.rs
+tests/ui/rfcs/rfc-2396-target_feature-11/issue-108655-inline-always-closure.rs
tests/ui/rust-2018/uniform-paths/from-decl-macro.rs
tests/ui/rust-2024/unsafe-extern-blocks/extern-items.rs
tests/ui/rust-2024/unsafe-extern-blocks/safe-items.rs
@@ -261,6 +345,7 @@
tests/ui/specialization/specialization-projection-alias.rs
tests/ui/specialization/specialization-projection.rs
tests/ui/specialization/transmute-specialization.rs
+tests/ui/stability-attribute/auxiliary/lint-stability.rs
tests/ui/structs-enums/numeric-fields.rs
tests/ui/structs-enums/struct-aliases.rs
tests/ui/structs/default-field-values/use-normalized-ty-for-default-struct-value.rs
@@ -328,7 +413,9 @@
tests/ui/trivial-bounds/trivial-bounds-object.rs
tests/ui/try-trait/yeet-for-option.rs
tests/ui/try-trait/yeet-for-result.rs
+tests/ui/type-alias-impl-trait/issue-53678-coroutine-and-const-fn.rs
tests/ui/type-alias-impl-trait/issue-57611-trait-alias.rs
+tests/ui/type-alias-impl-trait/issue-58662-coroutine-with-lifetime.rs
tests/ui/type_length_limit.rs
tests/ui/typeck/auxiliary/tdticc_coherence_lib.rs
tests/ui/underscore-imports/hygiene-2.rs
|
@m4rch3n1ng amazing, thank you so much for having a look! It seems that there are quite a few more places where outer annotations are allowed. Your method of comparing to rustc's behaviour is obviously much more fruitful than my attempt at following the specs. I am keen to adjust my PR accordingly to add outer attributes in more places as needed. Do you think that's a sensible approach, or would you avoid this move for other reasons? |
|
i think it should be doable, but i don't know if it's "worth it". it's not really tree-sitters job to like check if the rust code is completely valid, it should just be able to highlight it. i could see it being useful for #246, where it could potentially help with associating doc comments with the types they belong to, but even that use case is very niche and i think should only be considered if someone actually needs it for something, and i don't even really see an an actual use case for this pr. especially because it will make highlighting worse in some cases: if i write a for this reason there is quite a bit more syntax that tree-sitter accepts, that is also not valid rust syntax where i don't think it would be of any benefit to restrict them exactly to where they would be valid, because those errors make more sense to be reported by rust-analyzer instead of trying to handle them in tree-sitter and breaking syntax highlighting. |
|
Sure, I can expand on the motivation for being more faithful to the Rust syntax. It is mostly useful outside of the highlighting use case indeed. For instance, ast-grep uses tree-sitter to do syntax aware search and replace on source code, letting search patterns capture entire AST nodes. In this context, being able to reliably capture an entire declaration with the outer attributes that are attached to it would be quite useful. I do recognize that tree-sitter is primarily developed for syntax highlighting, so perhaps fidelity of the AST structures isn't actually a priority. Perhaps there's a case for forking this grammar to tweak it for those different use cases where fidelity is more important.
Isn't that something that happens all the time when parsing incomplete input though? I'd expect it to parse the attribute declaration generate a |
Fixes #244.
I reviewed which syntax elements outer attributes can be applied to: https://doc.rust-lang.org/reference/attributes.html.
I am not sure about
let_item: I couldn't find the corresponding item declaration in this document. Perhaps it's something that has been added recently and this document hasn't been updated yet (as the preamble states). It would be good if someone with a better knowledge of the language could confirm that. In doubt, I think the safe way is to allow them.