Remove `name_or_empty` #139615

@@ -561,12 +561,15 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
        allowed_target: Target,
    ) {
        if target != allowed_target {
+            let path = attr.path();
+            let path: Vec<_> = path.iter().map(|s| s.as_str()).collect();
+            let attr_name = path.join("::");


if this happens a lot throughout the compiler it's a bit ugly to keep iterating and joining these. In HIR I think I had a solution for this, with hir::AttributePath which implements Display. I'll add this to my own TODO list, to make this nicer at the same time when AttributeExt is removed :)

jdonszelmann · 2025-04-12T16:06:04Z

compiler/rustc_ast_passes/src/ast_validation.rs

@@ -347,7 +347,7 @@ impl<'a> AstValidator<'a> {
                    sym::forbid,
                    sym::warn,
                ];
-                !arr.contains(&attr.name_or_empty()) && rustc_attr_parsing::is_builtin_attr(*attr)
+                !attr.has_any_name(&arr) && rustc_attr_parsing::is_builtin_attr(*attr)


I like has_any_name, good change :)

jdonszelmann · 2025-04-12T16:10:35Z

compiler/rustc_hir_typeck/src/method/probe.rs

@@ -2346,8 +2346,9 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
        let hir_id = self.fcx.tcx.local_def_id_to_hir_id(local_def_id);
        let attrs = self.fcx.tcx.hir_attrs(hir_id);
        for attr in attrs {
-            if sym::doc == attr.name_or_empty() {
-            } else if sym::rustc_confusables == attr.name_or_empty() {
+            if attr.has_name(sym::doc) {


all these if chains are terribly when you're changing things, who knows whether things are exhaustive or not. They should be matches. But that's by no means your fault, it just reminds me again how bad this is...

nnethercote · 2025-04-13T11:28:16Z

Thanks for the review comments. I don't think any of them require action, are there more to come?

jdonszelmann · 2025-04-14T15:57:05Z

there are more to come, paused my review halfway through but just wanted to comment what there was already. sorry for the confusion

jdonszelmann · 2025-04-14T15:59:54Z

compiler/rustc_mir_dataflow/src/framework/graphviz.rs

@@ -144,9 +144,10 @@ impl RustcMirAttrs {
        attr: &ast::MetaItemInner,
        mapper: impl FnOnce(Symbol) -> Result<T, ()>,
    ) -> Result<(), ()> {
+        // Unwrapping the name is safe because this is only called when `has_name` has succeeded.


it could be nicer to just pass the name in that case, to make sure this can never go wrong

Ok, I added a new commit doing this.

jdonszelmann · 2025-04-14T16:06:11Z

compiler/rustc_passes/src/errors.rs

    #[primary_span]
    pub span: Span,
-    pub name: Symbol,


it's a little unfortunate that the symbol is removed from the error here now. In another case earlier I thought it didn't matter so much but having the name there can be nice during testing, i.e. the main error string will list the attribute that's an error. If there are multiple errors on one line, it's afaik hard to deduce which one was the error based just on the span.

though it does not seem to affect any test right now, which is nice I guess. What do you think?

There are a few cases like this. E.g. for this code:

#[macro_export(not_local_inner_macros)]

The old error was:

error: `not_local_inner_macros` isn't a valid `#[macro_export]`

and the new error is:

invalid `#[macro_export]` argument

which doesn't mention the invalid argument, because that argument might be a non-identifer, e.g. a string literal.
It could be made to mention it, it's requires an extra case, which is a little more code and complexity, and didn't seem worth it for these quite obscure error messages.

hmm, no I do agree the error is still perfectly clear for users and the value might be small

jdonszelmann · 2025-04-16T07:09:35Z

alright! very nice this, r=me then :)

nnethercote · 2025-04-16T07:44:43Z

Thanks!

@bors r=jdonszelmann

bors · 2025-04-16T07:44:46Z

📌 Commit 20d7373 has been approved by jdonszelmann

It is now in the queue for this repository.

…onszelmann Remove `name_or_empty` Another step towards rust-lang#137978. r? `@jdonszelmann`

bors · 2025-04-16T23:15:12Z

☔ The latest upstream changes (presumably #139912) made this pull request unmergeable. Please resolve the merge conflicts.

This adds two new warnings, both of which print the attribute incorrectly as `#[]`. The next commit will fix this.

The current code assumes that the attribute is just an identifier, and so misprints paths.

This shows places where the use of `name_or_empty` causes problems, i.e. we print empty identifiers in error messages: ``` error: unrecognized field name `` error: `` isn't a valid `#[macro_export]` argument `#[no_sanitize()]` should be applied to a function ``` (The last one is about an attribute `#[no_sanitize("address")]`.) The next commit will fix these.

I'm removing empty identifiers everywhere, because in practice they always mean "no identifier" rather than "empty identifier". (An empty identifier is impossible.) It's better to use `Option` to mean "no identifier" because you then can't forget about the "no identifier" possibility. Some specifics: - When testing an attribute for a single name, the commit uses the `has_name` method. - When testing an attribute for multiple names, the commit uses the new `has_any_name` method. - When using `match` on an attribute, the match arms now have `Some` on them. In the tests, we now avoid printing empty identifiers by not printing the identifier in the `error:` line at all, instead letting the carets point out the problem.

nnethercote · 2025-04-17T00:04:02Z

I rebased.

@bors r=jdonszelmann

bors · 2025-04-17T00:04:04Z

📌 Commit 846c10f has been approved by jdonszelmann

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#139084 (hygiene: Rename semi-transparent to semi-opaque) - rust-lang#139236 (Use a session counter to make anon dep nodes unique) - rust-lang#139650 (Fix `register_group_alias` for tools) - rust-lang#139770 (Rename `LifetimeName` as `LifetimeKind`.) - rust-lang#139846 (Remove `kw::Empty` uses in rustdoc) - rust-lang#139891 (Include optional dso_local marker for functions in `enum-match.rs`) - rust-lang#139908 (parser: Remove old diagnostic notes for type ascription syntax) - rust-lang#139917 (fix for multiple `#[repr(align(N))]` on functions) Failed merges: - rust-lang#139615 (Remove `name_or_empty`) r? `@ghost` `@rustbot` modify labels: rollup

…onszelmann Remove `name_or_empty` Another step towards rust-lang#137978. r? `@jdonszelmann`

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#138934 (support config extensions) - rust-lang#139114 (Implement `pin!()` using `super let`) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

…onszelmann Remove `name_or_empty` Another step towards rust-lang#137978. r? `@jdonszelmann`

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#138934 (support config extensions) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#139615 - nnethercote:rm-name_or_empty, r=jdonszelmann Remove `name_or_empty` Another step towards rust-lang#137978. r? ``@jdonszelmann``

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138528 (deref patterns: implement implicit deref patterns) - rust-lang#139393 (rustdoc-json: Output target feature information) - rust-lang#139553 (sync::mpsc: prevent double free on `Drop`) - rust-lang#139615 (Remove `name_or_empty`) - rust-lang#139853 (Disable combining LLD with external llvm-config) - rust-lang#139913 (rustdoc/clean: Fix lowering of fn params (fixes correctness & HIR vs. middle parity regressions)) - rust-lang#139942 (Ignore aix for tests/ui/erros/pic-linker.rs) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned jdonszelmann Apr 10, 2025

nnethercote mentioned this pull request Apr 10, 2025

Too much kw::Empty usage #137978

Open

jdonszelmann reviewed Apr 12, 2025

View reviewed changes

jdonszelmann reviewed Apr 14, 2025

View reviewed changes

nnethercote force-pushed the rm-name_or_empty branch from 67d536a to 20d7373 Compare April 15, 2025 23:40

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2025

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 16, 2025

Rollup merge of rust-lang#139615 - nnethercote:rm-name_or_empty, r=jd…

8150743

…onszelmann Remove `name_or_empty` Another step towards rust-lang#137978. r? `@jdonszelmann`

This was referenced Apr 16, 2025

Rollup of 9 pull requests #139927

Closed

Rollup of 8 pull requests #139938

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 16, 2025

Warnings-as-errors in check-builtin-attr-ice.rs.

4be670f

This adds two new warnings, both of which print the attribute incorrectly as `#[]`. The next commit will fix this.

nnethercote added 4 commits April 17, 2025 09:50

Fix attribute printing in an error.

400e8e5

The current code assumes that the attribute is just an identifier, and so misprints paths.

Avoid an unwrap in RustcMirAttrs::set_field.

846c10f

nnethercote force-pushed the rm-name_or_empty branch from 20d7373 to 846c10f Compare April 17, 2025 00:03

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2025

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 17, 2025

Rollup merge of rust-lang#139615 - nnethercote:rm-name_or_empty, r=jd…

e4a8af0

…onszelmann Remove `name_or_empty` Another step towards rust-lang#137978. r? `@jdonszelmann`

matthiaskrgr mentioned this pull request Apr 17, 2025

Rollup of 9 pull requests #139957

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 17, 2025

Rollup merge of rust-lang#139615 - nnethercote:rm-name_or_empty, r=jd…

f6008db

…onszelmann Remove `name_or_empty` Another step towards rust-lang#137978. r? `@jdonszelmann`

matthiaskrgr mentioned this pull request Apr 17, 2025

Rollup of 8 pull requests #139980

Closed

matthiaskrgr mentioned this pull request Apr 18, 2025

Rollup of 7 pull requests #139996

Merged

bors merged commit 540fb22 into rust-lang:master Apr 18, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone Apr 18, 2025

matthiaskrgr mentioned this pull request Apr 20, 2025

ICE: None in check_attr #140082

Open

nnethercote deleted the rm-name_or_empty branch April 20, 2025 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `name_or_empty` #139615

Remove `name_or_empty` #139615

nnethercote commented Apr 10, 2025

rustbot commented Apr 10, 2025

nnethercote commented Apr 10, 2025

jdonszelmann commented Apr 10, 2025

jdonszelmann Apr 12, 2025

jdonszelmann Apr 12, 2025

jdonszelmann Apr 12, 2025

nnethercote commented Apr 13, 2025

jdonszelmann commented Apr 14, 2025

jdonszelmann Apr 14, 2025

nnethercote Apr 15, 2025

jdonszelmann Apr 14, 2025

jdonszelmann Apr 14, 2025

nnethercote Apr 14, 2025

jdonszelmann Apr 15, 2025

jdonszelmann commented Apr 16, 2025

nnethercote commented Apr 16, 2025

bors commented Apr 16, 2025

bors commented Apr 16, 2025

nnethercote commented Apr 17, 2025

bors commented Apr 17, 2025

Remove name_or_empty #139615

Remove name_or_empty #139615

Conversation

nnethercote commented Apr 10, 2025

rustbot commented Apr 10, 2025

nnethercote commented Apr 10, 2025

jdonszelmann commented Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Apr 13, 2025

jdonszelmann commented Apr 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdonszelmann commented Apr 16, 2025

nnethercote commented Apr 16, 2025

bors commented Apr 16, 2025

bors commented Apr 16, 2025

nnethercote commented Apr 17, 2025

bors commented Apr 17, 2025

Remove `name_or_empty` #139615

Remove `name_or_empty` #139615