Fix clippy *errors* in current build #7108
Conversation
| self.name == other.name && Arc::ptr_eq(&self.expander, &other.expander) | ||
| let thisptr = self.expander.as_ref() as *const dyn ProcMacroExpander as *const u8; | ||
| let otherptr = other.expander.as_ref() as *const dyn ProcMacroExpander as *const u8; | ||
| self.name == other.name && std::ptr::eq(thisptr, otherptr) |
There was a problem hiding this comment.
oh dear... rust-lang/rust#80505
There was a problem hiding this comment.
There's a lot of nasty in pointer comparisons here I guess, though once you start transmuting stuff I think I tap out on knowing what's sane 😬
Do you mean, by this comment, that you want this particular change dropped, annotated that at some point it might be revertable, or do you just mean to mention the horror 😁 ?
| @@ -77,7 +77,7 @@ impl FromStr for TokenStream { | |||
| /// with `Delimiter::None` delimiters and negative numeric literals. | |||
| impl fmt::Display for TokenStream { | |||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
| f.write_str(&self.to_string()) | |||
| f.write_str(&self.0.to_string()) | |||
There was a problem hiding this comment.
I recommend that do not changes this file as it is almost straight copy from rustc such that it is quite hard to trace if they changed something relevant.
There was a problem hiding this comment.
I understand, however without this change, afaict the Display impls are recursive because the blanket ToString impl calls the Display impl a'la https://doc.rust-lang.org/stable/src/alloc/string.rs.html#2194-2207
There was a problem hiding this comment.
Yes, but these won't be called in normal situation and these implementation are just dummy for make the ABI happy.
Anyway, I think it is okay to change it for functionality
There was a problem hiding this comment.
My fear is that if they're there for ABI compatibility then it implies they could be called at which point they'd break. If on the other hand they can't be called then removing them (commenting them out?) might be a better bet since it'd be clearer what was going on. I'm not clear about how that might affect compatibility though, you're the expert here :D
There was a problem hiding this comment.
Okay, Agreed. Maybe add a comment to indicate it is a complicated case :)
I'm not clear about how that might affect compatibility though, you're the expert here :D
I wish I were the expert. :( I even don't understand why the original rustc allow duplication implementation of the ToString It should be conflicted to impl<T: fmt::Display + ?Sized> ToString implementation :
There was a problem hiding this comment.
I think that when the real code is built, specialisation is possible. I'll consider a suitable comment to add
|
@kinnison our Clippy policy is documented here, btw: https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/style.md#clippy |
In the proc-macro bridge code, we have to disable a number of ToString implementations which results in incorrectly implemented Display implementations. This works around being unable to have the ToString implementations and corrects a potential infinite loop in the Display implementations as a result. Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
In theory the failure-case Arc::ptr_eq() on dyn Trait would not happen in the current codebase, however this conversion approach is recommended for checking instance pointers properly. Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
This makes the two match arms look more similar and removes a warning for drop(&mut T) being pointless. Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
The bodies of the if ladder here only differ when in test mode. This attribute silences clippy's error here in non-test builds. Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
I managed to miss that when I went looking, thanks for the pointer. I think I only "corrected" things which were actively wrong, but if others don't agree then I'm happy to just close this PR now that I know the intent is to not worry overly about clippy. |
|
Based on #7112 going in, which would fail another of my commits out, I'll just close this off for now and maybe look at cleaning bits up another time, thanks anyway. 👍 |
Given the large number of clippy warnings present in master, I'm assuming there's not a general intention for rust-analyzer to be clippy-clean, however the fixes in this branch deal with things clippy considers an error and which therefore cause vscode to light up red if using clippy by default.
I don't believe the changes to be particularly controversial, and I stopped short of trying to clean up all the clippy warnings and lints.
The only commit I'm not 100% certain on is the proc_macro_srv change since that's obviously delicate stuff. I believe it to be right, but I'm unsure where to insert tests to confirm compatibility hasn't been disrupted.