Snippet rework #14724
Snippet rework #14724
Conversation
|
r? @Alexendoo rustbot has assigned @Alexendoo. Use |
rustbot
added
the
S-waiting-on-review
|
The first two commits are mainly renames. The third commit has all the actual changes. |
Jarcho
force-pushed
the
source_rework
branch
2 times, most recently
from
91b8913 to
b336277
Compare
|
Looking at Why is this not part of rustc in the first place? |
|
☔ The latest upstream changes (possibly 56f0182) made this pull request unmergeable. Please resolve the merge conflicts. |
clippy_utils/src/source.rs
Outdated
| /// Handle to a source file's text and a range within that file. | ||
| /// | ||
| /// With debug assertions the range is checked to be a valid substring of the source text. Without | ||
| /// assertions `None` will be returned from various functions when accessing the substring of the | ||
| /// source text fails. | ||
| #[derive(Clone)] | ||
| pub struct SourceFileRange { | ||
| file: SourceText, | ||
| range: Range<RelativeBytePos>, | ||
| } |
There was a problem hiding this comment.
Both SourceFileRange and SourceText can represent subsets of a source file, could they be merged into a single type?
There was a problem hiding this comment.
Doing so would require that SourceFileRange always validate it's range. The main point of SourceText is that it's definitely a valid string.
They're also serving two different purpose. One is a substring of the source text, the other is a movable view of a whole file.
There was a problem hiding this comment.
Always validating doesn't seem so bad to me, API wise it's some extra ?ing when using with_lo/with_hi
There was a problem hiding this comment.
They're still modelling two completely different things. Even if only one of them is exposed using both to implement it is still useful.
clippy_lints/src/ranges.rs
Outdated
| && let Some(span) = new_lhs.span.map_span(cx, |file| { | ||
| let src = file.with_hi(span.hi()).src_text()?; | ||
| // Do not continue if we have mismatched number of parens, otherwise the suggestion is wrong | ||
| src.matches('(').count() == src.matches(')').count() | ||
| (src.matches('(').count() == src.matches(')').count()).then_some(file) | ||
| }) |
There was a problem hiding this comment.
with_hi mutating file here is confusing, there's similar elsewhere with trim_start/trim_end
Reusing names of non mutating methods while also returning a value makes it difficult to realise what's happening
There was a problem hiding this comment.
Yeah, those need to be renamed.
Jarcho
force-pushed
the
source_rework
branch
4 times, most recently
from
ae6dc9d to
8fe48da
Compare
This comment has been minimized.
This comment has been minimized.
rustbot
added
has-merge-commits
|
Lintcheck changes for f0d6813
This comment will be updated if you push new changes |
Jarcho
added
the
S-blocked
This is blocking #14724 changelog: none
* Don't search spans between items for comments. * Don't lint items inside bodies, automatically derived impls. * Don't lint items inside macro generated modules unless they are namable outside. * Delay `is_from_proc_macro` checks * Handle reexports when checking if an item is reachable from the crate root. * Don't lint the crate root.
rustbot
removed
S-waiting-on-author
|
Should be ready, but still waiting on #14741 to be merged. Recent changes:
I'm still very much against trying to merge |
This is a further rework of our snippet accessing/creating code.
The general design goals with this:
The main implementation is split across two types.
SourceTextwhich holds theArc<SourceFile>and a pointer to the text. This type guarantees that the source is accessible and also implementsDisplay,Deref<Target=str>and others.SourceFileRangewhich holds aSourceTextinstance over the whole file and a sub-range in that file which can be manipulated. All span editing functions are on this type as well as conversion back into aSpan.The number of functions on
SpanExt(formerlySpanRangeExt) has been cut down to four.get_source_rangeis not actually used, but remains since it's the most general way to access everything and may end up being needed. All the span editing functions have been moved ontoSourceFileRangewith only a singlemap_spanfunction in their place. This should make span edits a little more composable.Without debug assertions,
Nonewill be returned when an invalid span is used to index the source text. With debug assertions all spans and span changed are asserted for validity (can be used to index the source in a single file). In both cases the source being unavailable will returnNone.changelog: None