-
Notifications
You must be signed in to change notification settings - Fork 430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust Analyzer support #227
Conversation
Can you share what IDE you're using to develop Rust in Linux? Does auto-completion etc work? |
Rust analyzer works with any LSP compatible editor. See https://rust-analyzer.github.io/manual.html#installation on how to set it up for many common code editors. Rust analyzer has support for auto-completion, go to definition, showing compilation errors (only some errors builtin to rust-analyzer work when not using cargo though) and has support for a couple of refactorings and more. |
Currently {
"rust-analyzer.linkedProjects": [
"/persist/data/kloenk/proj/linux/build/rust-project.json"
]
} proc_macro does not really work. This does results in not so great support inside of drivers for autocompletion. Autocompletion and got to definition works in the kernel crate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating the documentation seems like a good idea. I think we could remove the example rust-project.json from the documentation, as this creates a rust-project.json from the current .config file. |
Pinging @adamrk since he introduced it. |
Yeah we should definitely just get rid of the example if we're generating it. |
Rebased, removed the |
Looks good |
scripts/generate_rust_analyzer.sh
Outdated
lib_src=$3 | ||
bindgen_file=$4 | ||
|
||
dylib_and_extra=',"proc_macro_dylib_path":"./rust/libmodule.so","env":{"RUST_BINDINGS_FILE":"'$bindgen_file'"}, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not completely sure. I don't think so, as kernel reexports those macros, for example every driver uses module, which is in that dylib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, at least on my machine a config like this also seems to properly evaluate the proc-macro (e.g. __nvme_rust_author
can be found in the autocompletion and the incorrect-ident-case diagnostic fires inside the generated module!
code):
{
"sysroot_src": "/home/bobo1239/.rustup/toolchains/nightly-2021-02-20-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library",
"crates": [
{
"root_module": "rust/module.rs",
"edition": "2018",
"deps": [],
"proc_macro_dylib_path": "rust/libmodule.so"
},
{
"root_module": "rust/kernel/lib.rs",
"edition": "2018",
"deps": [
{
"crate": 0,
"name": "module"
}
]
},
{
"root_module": "drivers/nvme_rust/nvme.rs",
"edition": "2018",
"deps": [
{
"crate": 0,
"name": "module"
},
{
"crate": 1,
"name": "kernel"
}
],
"env": {"RUST_MODFILE": "???"}
}
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will remove the dylib for the normal modules again. I would still like to keep the custom alloc and core, to make changing them later easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding the env variables to every crate (even core and alloc) I don't see a harm in that, and it makes it easier.
Something's not quite right yet or I'm misusing Kbuild... When I run the
Trying inside the
If I change |
8e95c82
to
59dcd13
Compare
that's pretty sure an error. It probably works on my side, as I'm using |
scripts/generate_rust_analyzer.sh
Outdated
\"cfg\": $cfg, | ||
\"env\": { | ||
\"RUST_BINDGEN_FILE\": \"$bindgen_file\", | ||
\"RUST_MODFILE\": \"compile_error!(\\\"This is only for rust-analyzer\\\");\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to change the RUST_MODFILE
value to something else since this gets transformed to
#[cfg(not(MODULE))]
#[link_section = ".modinfo"]
#[used]
pub static __rust_random_file: [u8; 67] = *b"rust_random.file=compile_error!("This is only for rust-analyzer");\0";
which is invalid (notice the "
) and causes rust-analyzer to choke:
Options are "upgrading" from \\\
to \\\\\\\
(+4 since we're escaping through three levels...) or probably better to just use "RUST_MODFILE": "This is only for rust-analyzer"
which should then greet you with a diagnostic:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, missed that. It also does not show that on my side. Not sure why I don't get that error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, there's a setting called rust-analyzer.procMacro.enable
but it should already be enabled by default...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the values generated by the procMarco. But I don't see warnings from the Marco itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my bad. I've picked up the Error Lens VS Code extension from one of the rust-analyzer changelog which renders the diagnostics like that. You should still be able to see the messages by hovering over module!
and there should also be a small yellow dashed line below the m
as in my screenshots above.
scripts/generate_rust_analyzer.sh
Outdated
\"is_workspace_member\": $member, | ||
\"cfg\": $cfg, | ||
\"env\": { | ||
\"RUST_BINDGEN_FILE\": \"$bindgen_file\", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\"RUST_BINDGEN_FILE\": \"$bindgen_file\", | |
\"RUST_BINDINGS_FILE\": \"$bindgen_file\", |
After correcting the env var and adding something like
"source": {
"include_dirs": ["/home/bobo1239/Development/Rust/linux/rust"],
"exclude_dirs": []
},
to the kernel
crate in rust-project.json
, autocomplete works with the generated bindings. (it is somewhat slow though...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, the include_dirs
is needed for rust analyzer to include the generated bindings file to its virtual file system. By default it only considers ${srcdir}/rust/kernel
since that's the directory where the kernel
crate root lies.
I've tried an out-of-tree build now and have noticed that my last "source"
suggestion wasn't quite correct. It should instead be:
"source": {
"include_dirs": ["${srctree}/rust/kernel", "${objtree}/rust"],
"exclude_dirs": []
},
This will include the sources for kernel
and also the generated bindings file in the objtree
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try to commit that today. Is that only for the kernel crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since that is the crate which includes the RUST_BINDINGS_FILE
. Also we are actually forced to do this only for the kernel
crate since the following is stated in the RA documentation:
/// Different crates can share the same `source`.
///
/// If two crates share an `.rs` file in common,
/// they *must* have the same `source`.
/// rust-analyzer assumes that files from one
/// source can't refer to files in another source.
Hopefully fixed everything now. |
Yup, works fine for me now. Thanks! |
I'm finally finished with my finals (at least for a month). I rewrote it in python, I think it has a better readability. @Bobo1239 what is your |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for rewriting that in Python! It is indeed much more readable (and reliable -- if only thanks to the json
built-in module, proper strings, paths and error handling in the language, etc.).
Let me give you a few suggestions around some Python features and built-in modules that help a lot for many of these things (I am assuming you were more or less new to Python). I hope it is useful for you!
Do not feel like you have to change everything in this PR, though.
scripts/generate_rust_analyzer.py
Outdated
if (len(sys.argv) != 5): | ||
print("Not enough arguments") | ||
sys.exit(-1) | ||
|
||
srctree = sys.argv[1] | ||
objtree = sys.argv[2] | ||
lib_src = sys.argv[3] | ||
bindgen_file = sys.argv[4] | ||
|
||
crate_list = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest creating a main()
, called from __main__
and moving all non-constant globals and otherwise "executed" code inside there (i.e. it is best not to have lines that "run" things, and only "define", e.g. def
, class
).
This requires removing default parameters from generate_kernel()
, but it is a good idea anyway.
Furthermore, try argparse
-- it is a bit verbose and I never remember the details (I usually copy-paste the basic usage from project to project), but it does everything you need for CLI argument handling.
In this case, you need to write something like:
parser = argparse.ArgumentParser(description='Foo')
parser.add_argument('--verbose', '-v', action='store_true', help='Verbose')
parser.add_argument('srctree', help='bar')
parser.add_argument('objtree', help='baz')
# ...
args = parser.parse_args()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebuild it somehow with a class. not sure if it is better. maybe take a look :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... what does the class buy us here? i.e. the globals are still globals etc.
scripts/generate_rust_analyzer.py
Outdated
def get_default_deps(source = crate_list): | ||
return generate_deps(source, ["core", "alloc", "kernel"]) | ||
|
||
def generate_crate(name, module, member = True, deps = get_default_deps(), cfg = generate_cfgs(), extra = {}, edition = "2018"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful with mutable default parameters (and function calls). In general, in Python, one only uses immutable default parameters (literals, typically) -- anything else should be None
and handled in the body of the function.
The reason is that default parameters are evaluated once when the function definition is encountered and references to objects kept, rather than re-evaluated every time they are called, e.g. this:
def f(l = []):
l.append(42)
print(l)
f()
f()
Prints:
[42]
[42, 42]
(Yeah, Python got this wrong, it is a major pitfall)
Same for get_default_deps()
etc. which take a global mutable default parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed that with the classy way
scripts/generate_rust_analyzer.py
Outdated
return pos | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid returning an empty value, you can do something similar to a Rust panic, e.g.:
raise RuntimeError("Not found")
or similar outside of the loop. You can also provide more details in the error easily using an f-string (see the other comments first).
scripts/generate_rust_analyzer.py
Outdated
ret = [] | ||
for x in deps: | ||
ret.append({"crate": find(source, x), "name": x}) | ||
return ret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually avoid comprehensions when things are complex, but this is pretty much the best case for them:
ret = [] | |
for x in deps: | |
ret.append({"crate": find(source, x), "name": x}) | |
return ret | |
return [{"crate": find(source, x), "name": x} for x in deps] |
Another option is map()
with a lambda etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use that option. Don't know how to use map, and I think this way is readable enough.
scripts/generate_rust_analyzer.py
Outdated
makefile = path.dirname(filepath) + "/Makefile" | ||
name = file.replace('.rs', '') | ||
objname = name + ".o" | ||
print("checking " + objname, file=sys.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For templating, avoid string concatenation. Instead, there is the %
operator (in the past, for simple cases), the format()
method (for bigger/complex cases) and nowadays, f-strings (similar to format()
but for the inline cases too, better than the %
operator):
a = 42
'hello %s there' % a # the % operator
'hello {a} there'.format(a=a) # the .format() method
f'hello {a} there' # f-strings
Furthermore, for stderr
logging, give logging
a try instead:
logging.info("hello %s there", a)
For small scripts, I usually configure its output in main()
as:
logging_level = logging.DEBUG if verbose else logging.INFO
logging.basicConfig(format='[%(asctime)s] [%(levelname)s] %(message)s', level=logging_level)
Which gives this nice log:
[2021-05-05 14:38:17,325] [INFO] hello 42 there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't logging a bit much for this small (mostly) one shot script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you write a lot of similar Python scripts, you realize it is easier to just always use argparse
, logging
and friends rather than "upgrade" them later, and so one ends up cp
ing a template.py
file with the handful lines of setup every time you write a script :-)
But yeah, without context, it can be seen as overkill.
scripts/generate_rust_analyzer.py
Outdated
import os | ||
from os import path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nowadays there is pathlib
which is nicer, give it a look!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it works. I don't want to learn python now. I'm doing this project for rust :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, as mentioned above, feel free to leave it as it is.
AFAIU, it is easy to test a new version by comparing old vs. new output, right? So someone that wants to clean it up should not have a problem testing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should work for testing. I just restarted rust-analyzer which each new try
scripts/generate_rust_analyzer.py
Outdated
) | ||
|
||
|
||
def find_rs(tree): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A feature you may like for find_rs
is generators, i.e. using yield
to return values one-by-one instead of returning a full list, similar to iterators in other languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how I would implement this
Hope your finals went well. I've tested both in-tree and out-of-tree builds and everything seems to work correctly so 👍 from me. Co-author line would be |
scripts/generate_rust_analyzer.py
Outdated
"crates": crate_list, | ||
"sysroot_src": lib_src | ||
} | ||
print(json.dumps(rust_project)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty printing is trivial now that we use Python's json
and is better to debug the file and for human readers etc.:
print(json.dumps(rust_project)) | |
print(json.dumps(rust_project, sort_keys=True, indent=4)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sort_keys safe? (guess it is, but would be fatal if it reorders the crates array)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is for dictionary keys, not for the ordered lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, if you want to avoid the intermediate string step etc., you can also write directly to stdout
:
json.dump(rust_project, sys.stdout, sort_keys=True, indent=4)
a3aff78
to
8ea93da
Compare
OK, let's do one thing, let me clean it up (which is easy for me now since it is in Python), and you can send it in this PR. Sounds good? |
Not sure what you mean with |
#!/usr/bin/env python3
"""generate_rust_analyzer - Generates the `rust-project.json` file for `rust-analyzer`.
"""
import argparse
import json
import logging
import pathlib
import sys
def generate_crates(srctree, objtree, sysroot_src, bindings_file):
# Generate the configuration list.
cfg = []
with open(objtree / "include" / "generated" / "rustc_cfg") as fd:
for line in fd:
line = line.replace("--cfg=", "")
line = line.replace("\n", "")
cfg.append(line)
# Now fill the crates list -- dependencies need to come first.
#
# Avoid O(n^2) iterations by keeping a map of indexes.
crates = []
crates_indexes = {}
def append_crate(display_name, root_module, is_workspace_member, deps, cfg):
crates_indexes[display_name] = len(crates)
crates.append({
"display_name": display_name,
"root_module": str(root_module),
"is_workspace_member": is_workspace_member,
"deps": [{"crate": crates_indexes[dep], "name": dep} for dep in deps],
"cfg": cfg,
"edition": "2018",
"env": {
"RUST_MODFILE": "This is only for rust-analyzer"
}
})
# First, the ones in `rust/` since they are a bit special.
append_crate(
"core",
sysroot_src / "core" / "src" / "lib.rs",
False,
[],
[],
)
append_crate(
"compiler_builtins",
srctree / "rust" / "compiler_builtins.rs",
True,
[],
[],
)
append_crate(
"alloc",
sysroot_src / "alloc" / "src" / "lib.rs",
False,
["core", "compiler_builtins"],
[],
)
append_crate(
"module",
srctree / "rust" / "module.rs",
True,
[],
[],
)
crates[-1]["proc_macro_dylib_path"] = "rust/libmodule.so"
append_crate(
"kernel",
srctree / "rust" / "kernel" / "lib.rs",
True,
["core", "alloc", "module"],
cfg,
)
crates[-1]["env"]["RUST_BINDINGS_FILE"] = str(bindings_file.resolve(True))
crates[-1]["source"] = {
"include_dirs": [
str(srctree / "rust" / "kernel"),
str(objtree / "rust")
],
"exclude_dirs": [],
}
# Then, the rest outside of `rust/`.
#
# We explicitly mention the top-level folders we want to cover.
for folder in ("samples", "drivers"):
for path in (srctree / folder).rglob("*.rs"):
logging.info("Checking %s", path)
name = path.name.replace(".rs", "")
# Skip those that are not crate roots.
if f"{name}.o" not in open(path.parent / "Makefile").read():
continue
logging.info("Adding %s", name)
append_crate(
name,
path,
True,
["core", "alloc", "kernel"],
cfg,
)
return crates
def main():
parser = argparse.ArgumentParser()
parser.add_argument('--verbose', '-v', action='store_true')
parser.add_argument("srctree", type=pathlib.Path)
parser.add_argument("objtree", type=pathlib.Path)
parser.add_argument("sysroot_src", type=pathlib.Path)
parser.add_argument("bindings_file", type=pathlib.Path)
args = parser.parse_args()
logging.basicConfig(
format="[%(asctime)s] [%(levelname)s] %(message)s",
level=logging.INFO if args.verbose else logging.WARNING
)
rust_project = {
"crates": generate_crates(args.srctree, args.objtree, args.sysroot_src, args.bindings_file),
"sysroot_src": str(args.sysroot_src),
}
json.dump(rust_project, sys.stdout, sort_keys=True, indent=4)
if __name__ == "__main__":
main() |
Notes on the script above:
- "root_module": "./rust/compiler_builtins.rs"
+ "root_module": "rust/compiler_builtins.rs"
rust-analyzer:
$(Q)$(srctree)/scripts/generate_rust_analyzer.py $(srctree) $(objtree) $(RUST_LIB_SRC) $(objtree)/rust/bindings_generated.rs > $(objtree)/rust-project.json
|
Does this script include |
Yeah, it does. It follows @kloenk's script (finds all Perhaps later on someone can take a look at reusing the |
What is this |
rust-analyzer works in my IDE with this change, pushed it, and added you as co-author |
scripts/generate_rust_analyzer.py
Outdated
json.dump(rust_project, sys.stdout, sort_keys=True, indent=4) | ||
|
||
if __name__ == "__main__": | ||
main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the newline at EOF. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem at all :P
https://www.gnu.org/software/make/manual/html_node/Secondary-Expansion.html I try to keep it as low as possible so that it only covers targets that need it (there are two that we could move up, but then it looks a bit awkward, so I left those two there). |
This target creates ${objtree}/rust-project.json which can be read by rust-analyzer to help with autocompletion for the kernel crate. The raw bindings do not work yet, as rust-analyzer seems to have a problem with the include macro. Co-authored-by: Boris-Chengbiao Zhou <bobo1239@web.de> Co-authored-by: Miguel Ojeda <ojeda@kernel.org> Signed-off-by: Finn Behrens <me@kloenk.de>
contains on #226