Migrate HasSource::source to return Option #7115
Conversation
| Either::Right(it) => FieldSource::Named(it), | ||
| }); | ||
| Some(field_source) | ||
| } |
There was a problem hiding this comment.
As a general comment, during similar refactors its better to delegate rather than duplicate: fn source() { Some(source_old()) }.
There was a problem hiding this comment.
Interesting, I went for duplicating because it made it easier to remove the old code when this is done.
| hir::AssocItem::TypeAlias(i) => ast::AssocItem::TypeAlias(i.source_old(db).value), | ||
| hir::AssocItem::Const(i) => ast::AssocItem::Const(i.source_old(db).value), | ||
| } | ||
| // Note: This throws away items with no source. |
There was a problem hiding this comment.
This is an interesting case! In the future(not this PR), we should add hir -> ast function which either uses an existing source, or just renders the hir directly.
There was a problem hiding this comment.
Yeah. I've seen reference to using hir for more in a few comments in the codebase.
| } | ||
|
|
||
| impl<D> ToNav for D | ||
| impl<D> TryToNav for D |
There was a problem hiding this comment.
(not this PR): might make sense to remove plain ToNav alltogether
There was a problem hiding this comment.
Yeah, that looks to be the case. Looking at the other implementations of ToNav whilst they won't panic, the NavigationTarget's name will be the empty string.
| let path = vfs.file_path(original_file); | ||
| let syntax_range = src.value.syntax().text_range(); | ||
| format_to!(msg, " ({} {:?})", path, syntax_range); | ||
| if let Some(src) = f.source(db) { |
nick96
force-pushed
the
migrate_hassource_source_return_option
branch
from
6115f27 to
47bffa0
Compare
nick96
changed the title
|
Hmm. I'm not sure why cross compilation is failing after changing to use $ docker run -v $PWD:/ws --workdir=/ws -it rust bash
$ rustup target add powerpc-unknown-linux-gnu x86_64-unknown-linux-musl
$ for target in powerpc-unknown-linux-gnu x86_64-unknown-linux-musl; do cargo check --target=$target --all-targets; done |
|
I think you need to rebase cause master has ConstParams now, so this impl is outdated when bors tries to merge https://github.com/rust-analyzer/rust-analyzer/blob/aa3ce16f2641b7eb562a8eae67738b0ff0c0b7b0/crates/hir/src/has_source.rs#L144-L150. Weird that only cross compilation fails though. Edit: This isnt bors failing yet, though you will run into this problem nevertheless. |
To start migrating HasSource::source to return an Option.
…os were special cased In rust-lang#6901 some special case handling for proc-macros was introduced to prevent panicing as they have no AST. Now the new HasSource::source method is used that returns an option. Generally this was a pretty trivial change, the only thing of much interest is that `hir::MacroDef` now implements `TryToNav` not `ToNav` as this allows us to handle `HasSource::source` now returning an option.
The `LifetimeParam` and `Local` variants use `source()` to find their range. Now that `source()` returns an `Option` we need to handle the `None` case.
…ot as simple as I thought
…tors optional They use source() which now returns an Option so they need to too.
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
nick96
force-pushed
the
migrate_hassource_source_return_option
branch
from
47bffa0 to
40cd6cd
Compare
|
Thanks @Veykril, that appears to have fixed it! Not sure why the cross compile job was trying to run that code though as it didn't exist on my branch. |
|
bors r+ cross build uses |
3 participants
I've made a start on fixing #6913 based on the provided work plan, migrating
HasSource::sourceto return anOption. The simple cases are migrated but there are a few that I'm unsure exactly how they should be handled:AnalysisStatsCmd::run: In verbose mode it includes the path to the module containing the function and the syntax range. I've handled this with an if-let but would it be better to blow up here withexpect? I'm not 100% on the code paths but if we're processing a function definition then the source should exist.I've handled
source()in all code paths asNonebeing a valid return value but are there some cases where we should just blow up? Also, all I've done is bubble up the returnedNones, there may be some places where we can recover and still provide something.