[PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Tamir Duberstein]

The macros crate has depended on core since its introduction in commit
1fbde52bde73 ("rust: add `macros` crate"). This dependency was omitted from
commit 8c4555ccc55c ("scripts: add `generate_rust_analyzer.py`") resulting in
false-positive warnings emitted from rust-analyzer such as:

  [{
  	"resource": "/Users/tamird/src/linux/rust/macros/module.rs",
  	"owner": "_generated_diagnostic_collection_name_#1",
  	"code": {
  		"value": "non_snake_case",
  		"target": {
  			"$mid": 1,
  			"path": "/rustc/",
  			"scheme": "https",
  			"authority": "doc.rust-lang.org",
  			"query": "search=non_snake_case"
  		}
  	},
  	"severity": 4,
  	"message": "Variable `None` should have snake_case name, e.g. `none`",
  	"source": "rust-analyzer",
  	"startLineNumber": 123,
  	"startColumn": 17,
  	"endLineNumber": 123,
  	"endColumn": 21
  }]

Add the missing dependency to improve the developer experience.

Fixes: 8c4555ccc55c ("scripts: add `generate_rust_analyzer.py`")
Suggested-by: Chayim Refael Friedman <chayimfr@gmail.com>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 scripts/generate_rust_analyzer.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
index aa8ea1a4dbe5..8f99af38dd09 100755
--- a/scripts/generate_rust_analyzer.py
+++ b/scripts/generate_rust_analyzer.py
@@ -75,7 +75,7 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
     append_crate(
         "macros",
         srctree / "rust" / "macros" / "lib.rs",
-        [],
+        ["core"],
         is_proc_macro=True,
     )
 

---
base-commit: 6273a058383e05465083b535ed9469f2c8a48321
change-id: 20250209-rust-analyzer-macros-core-dep-0f57868dd19f

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Tamir Duberstein]

[-cc Wedson Almeida Filho <wedsonaf@google.com>, retired email]

Actually macros depends on std and proc_macro rather than core.

On Sun, Feb 9, 2025 at 10:29 AM Tamir Duberstein <tamird@gmail.com> wrote:
>
> The macros crate has depended on core since its introduction in commit
> 1fbde52bde73 ("rust: add `macros` crate"). This dependency was omitted from
> commit 8c4555ccc55c ("scripts: add `generate_rust_analyzer.py`") resulting in
> false-positive warnings emitted from rust-analyzer such as:
>
>   [{
>         "resource": "/Users/tamird/src/linux/rust/macros/module.rs",
>         "owner": "_generated_diagnostic_collection_name_#1",
>         "code": {
>                 "value": "non_snake_case",
>                 "target": {
>                         "$mid": 1,
>                         "path": "/rustc/",
>                         "scheme": "https",
>                         "authority": "doc.rust-lang.org",
>                         "query": "search=non_snake_case"
>                 }
>         },
>         "severity": 4,
>         "message": "Variable `None` should have snake_case name, e.g. `none`",
>         "source": "rust-analyzer",
>         "startLineNumber": 123,
>         "startColumn": 17,
>         "endLineNumber": 123,
>         "endColumn": 21
>   }]
>
> Add the missing dependency to improve the developer experience.
>
> Fixes: 8c4555ccc55c ("scripts: add `generate_rust_analyzer.py`")
> Suggested-by: Chayim Refael Friedman <chayimfr@gmail.com>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  scripts/generate_rust_analyzer.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
> index aa8ea1a4dbe5..8f99af38dd09 100755
> --- a/scripts/generate_rust_analyzer.py
> +++ b/scripts/generate_rust_analyzer.py
> @@ -75,7 +75,7 @@ def generate_crates(srctree, objtree, sysroot_src, external_src, cfgs):
>      append_crate(
>          "macros",
>          srctree / "rust" / "macros" / "lib.rs",
> -        [],
> +        ["core"],
>          is_proc_macro=True,
>      )
>
>
> ---
> base-commit: 6273a058383e05465083b535ed9469f2c8a48321
> change-id: 20250209-rust-analyzer-macros-core-dep-0f57868dd19f
>
> Best regards,
> --
> Tamir Duberstein <tamird@gmail.com>
>

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Miguel Ojeda]

Hi Tamir,

Thanks for looking into rust-analyzer, it is great to get improvements
in this area.

On Sun, Feb 9, 2025 at 4:29 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> The macros crate has depended on core since its introduction in commit

To be clear, `macros` does not depend on the (kernel) `core`. But it
depends on host crates: not just `core`, but `proc_macro`, `std` etc.,
which is why we can use things like e.g. `std::fmt::Write`, right? I
don't know if the distinction matters for rust-analyzer, though, but
things like their `cfg`s may differ.

I also wonder if we should also be passing `std` etc. given it seems
we need to pass `core`. Does it handle correctly that `Write`, for
instance? Perhaps rust-analyzer allows to pass different sets of
crates for this, even if they share the same display name -- then we
could pass the right `cfg`s and so on.

Cc'ing Lukas and Danilo who have been looking into rust-analyzer
support recently (Fiona is already there).

One more quick comment: since this would apply to all proc macros,
should we do it for all proc macros? i.e. instead of hardcoding it
here, using instead the `is_proc_macro` tag we already have.

Cheers,
Miguel

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Tamir Duberstein]

Hi Miguel, thanks for the quick look!

On Sun, Feb 9, 2025 at 11:29 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Hi Tamir,
>
> Thanks for looking into rust-analyzer, it is great to get improvements
> in this area.
>
> On Sun, Feb 9, 2025 at 4:29 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > The macros crate has depended on core since its introduction in commit
>
> To be clear, `macros` does not depend on the (kernel) `core`.

You're right to point out the difference between `kernel::core`
(module) and `core` (crate). This confusion is solved below (because
it doesn't actually depend on the `core` crate).

> But it
> depends on host crates: not just `core`, but `proc_macro`, `std` etc.,
> which is why we can use things like e.g. `std::fmt::Write`, right? I
> don't know if the distinction matters for rust-analyzer, though, but
> things like their `cfg`s may differ.
>
> I also wonder if we should also be passing `std` etc. given it seems
> we need to pass `core`. Does it handle correctly that `Write`, for
> instance?

Yep, it matters! I realized the same in
https://lore.kernel.org/all/CAJ-ks9nkY7za5zQLv7YkvCeJ1BQ70ZnTTP3Q4VNAmMaG3sGu+Q@mail.gmail.com/.
With core replaced by std and proc_macro, `std::fmt::Write` and all
the `proc_macro` paths start working as well.

> Perhaps rust-analyzer allows to pass different sets of
> crates for this, even if they share the same display name -- then we
> could pass the right `cfg`s and so on.

I'm not exactly sure what you mean by different sets of crates here.
What `cfg`s would you want to pass?

> Cc'ing Lukas and Danilo who have been looking into rust-analyzer
> support recently (Fiona is already there).
>
> One more quick comment: since this would apply to all proc macros,
> should we do it for all proc macros? i.e. instead of hardcoding it
> here, using instead the `is_proc_macro` tag we already have.

We could, but then it's redundant with the deps section. I'll leave it
explicit for now, since it is just one line.

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Miguel Ojeda]

On Sun, Feb 9, 2025 at 5:39 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> I'm not exactly sure what you mean by different sets of crates here.
> What `cfg`s would you want to pass?

What I mean is that, in principle, `cfg`s can be different for the
kernel crates vs. the host crates. For instance, we use
`cfg(no_fp_fmt_parse)` for the kernel's `core`. In practice, it may
not matter. But, in theory, we should be passing different sets.

> We could, but then it's redundant with the deps section. I'll leave it
> explicit for now, since it is just one line.

I am not sure what you mean.

Cheers,
Miguel

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Tamir Duberstein]

On Sun, Feb 9, 2025 at 11:43 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sun, Feb 9, 2025 at 5:39 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > I'm not exactly sure what you mean by different sets of crates here.
> > What `cfg`s would you want to pass?
>
> What I mean is that, in principle, `cfg`s can be different for the
> kernel crates vs. the host crates. For instance, we use
> `cfg(no_fp_fmt_parse)` for the kernel's `core`. In practice, it may
> not matter. But, in theory, we should be passing different sets.

Ah, I see! I'm happy to do that in this patch if you think it's worth it.

> > We could, but then it's redundant with the deps section. I'll leave it
> > explicit for now, since it is just one line.
>
> I am not sure what you mean.

The python function `append_crate` takes both `deps` and
`is_proc_macro`, so implying dependencies based on the value of
`is_proc_macro` could create duplication.

> Cheers,
> Miguel

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Miguel Ojeda]

On Sun, Feb 9, 2025 at 5:49 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Ah, I see! I'm happy to do that in this patch if you think it's worth it.

I don't know if it is worth it (it requires testing it, I guess) -- we
can go simple until we see we need it, especially for a fix.

> The python function `append_crate` takes both `deps` and
> `is_proc_macro`, so implying dependencies based on the value of
> `is_proc_macro` could create duplication.

Yes, but the dependencies for proc macros are a completely different
set -- that is what I was discussing above. And those dependencies
will be shared across all proc macros, when/if we have several
eventually (e.g. per subsystem).

That is why, since we already have `is_proc_macro`, I think we should
just use that. It may make sense to split `append_crate` into two
functions or similar, e.g. having a `append_proc_macro_crate` (that
perhaps does not require any dependencies) -- it would be the same
idea but cleaner.

Anyway, the simpler the change, the better, especially for a fix. But
please take it into consideration.

Cheers,
Miguel

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Tamir Duberstein]

On Sun, Feb 9, 2025 at 12:05 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sun, Feb 9, 2025 at 5:49 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Ah, I see! I'm happy to do that in this patch if you think it's worth it.
>
> I don't know if it is worth it (it requires testing it, I guess) -- we
> can go simple until we see we need it, especially for a fix.
>
> > The python function `append_crate` takes both `deps` and
> > `is_proc_macro`, so implying dependencies based on the value of
> > `is_proc_macro` could create duplication.
>
> Yes, but the dependencies for proc macros are a completely different
> set -- that is what I was discussing above. And those dependencies
> will be shared across all proc macros, when/if we have several
> eventually (e.g. per subsystem).
>
> That is why, since we already have `is_proc_macro`, I think we should
> just use that. It may make sense to split `append_crate` into two
> functions or similar, e.g. having a `append_proc_macro_crate` (that
> perhaps does not require any dependencies) -- it would be the same
> idea but cleaner.
>
> Anyway, the simpler the change, the better, especially for a fix. But
> please take it into consideration.
>
> Cheers,
> Miguel

Heh, I keep typing a reply and you keep beating me to it. TL;DR I came
to the same conclusion: we should keep the fix simple and refactor on
-next.

I'll send v2 shortly.

The detailed reason why splitting host and target crates requires
refactoring is that we currently we use `display_name` as a crate's
unique identifier (i.e. both as a way of naming it as another crate's
dependency, and as its name in the dependent crate). If we wanted to
have separate target and host `core` crates we'd need a slightly more
sophisticated way of describing dependencies. I'll work on this in a
separate series.

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Miguel Ojeda]

On Sun, Feb 9, 2025 at 6:09 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> The detailed reason why splitting host and target crates requires
> refactoring is that we currently we use `display_name` as a crate's
> unique identifier (i.e. both as a way of naming it as another crate's
> dependency, and as its name in the dependent crate). If we wanted to
> have separate target and host `core` crates we'd need a slightly more
> sophisticated way of describing dependencies. I'll work on this in a
> separate series.

Yeah, that is why I was wondering if it would be worth it or not, and
why I asked Lukas et al. (who leads rust-analyzer) if rust-analyzer is
OK with that to begin with (or what is the expected way to do this),
i.e. it may be that rust-analyzer does not "like" several crates with
the same `display_name`.

In any case, I would like to keep the complexity of the script low if
possible unless it really makes rust-analyzer understand the code
better etc.

Thanks!

Cheers,
Miguel

Re: [PATCH] scripts: generate_rust_analyzer.py: add missing macros -> core dep

[Miguel Ojeda]

On Sun, Feb 9, 2025 at 5:39 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> You're right to point out the difference between `kernel::core`
> (module) and `core` (crate). This confusion is solved below (because
> it doesn't actually depend on the `core` crate).

By the way, I didn't read this sentence carefully before: I was not
talking about "`kernel::core` (module)" -- not sure what you mean by
that.

I am talking about the `core` crate in both cases. One is the one we
build for the kernel, and the other is the host one which is used by
proc macros.

The difference may not matter, but one is built differently than the
other, including the `cfg` set differences (and at least the `cfg` set
is used by rust-analyzer).

Moreover, I think it does depend on the `core` crate, at least
indirectly, since `std` depends on `core`.

Cheers,
Miguel