Add function name to the non local effect lint #1019
Conversation
There was a problem hiding this comment.
Thanks a lot for doing this, @fcasal. My comments are all very minor and nit-picky.
Do you think it would be difficult to include an example in the tests of why this was desired in the first place? You can say no, we can merge this, and leave that for another day.
| if_chain! { | ||
| if !contributing_calls.contains(index); | ||
| if let Some(call_span) = is_call_with_mut_ref(cx, mir, &path[i..]); | ||
| if let Some(func_and_span) = is_call_with_mut_ref(cx, mir, &path[i..]); |
There was a problem hiding this comment.
| if let Some(func_and_span) = is_call_with_mut_ref(cx, mir, &path[i..]); | |
| if let Some((func, span)) = is_call_with_mut_ref(cx, mir, &path[i..]); |
Style nit (saves having to write .0 and .1). Note that the code in the block would need to be adjusted.
| call_span, | ||
| "call with mutable reference before error return", | ||
| func_and_span.1, | ||
| format!("call to {} with mutable reference before error return", func_and_span.0).as_str(), |
There was a problem hiding this comment.
Style nit: &format!(...) is more concise than format!(...).as_str().
| @@ -223,7 +224,8 @@ fn is_call_with_mut_ref<'tcx>( | |||
| if locals.iter().any(|local| is_mut_ref_arg(mir, local)) | |||
| || constants.iter().any(|constant| is_const_ref(constant)); | |||
| then { | |||
| Some(*fn_span) | |||
| let func_string = format!("{func:#?}"); | |||
There was a problem hiding this comment.
It feels odd to me to convert something to a string using Debug formatting, only to later print it with Display formatting.
Could we instead change the return type to Option<(&'a Operand, Span)>, and include the operand directly in the warning using Debug formatting?
Also, I'm not certain, but I think just {:?} might work (i.e., instead of {:#?}).
| @@ -12,7 +12,7 @@ LL | Err(VarError::NotPresent) | |||
| = note: `-D non-local-effect-before-error-return` implied by `-D warnings` | |||
| = help: to override `-D warnings` add `#[allow(non_local_effect_before_error_return)]` | |||
|
|
|||
| error: call with mutable reference before error return | |||
| error: call to std::vec::Vec::<u32>::push with mutable reference before error return | |||
There was a problem hiding this comment.
| error: call to std::vec::Vec::<u32>::push with mutable reference before error return | |
| error: call to `std::vec::Vec::<u32>::push` with mutable reference before error return |
Since the function paths are technically syntax, could we enclose them in backticks (as shown)?
smoelius
force-pushed
the
function_info_non_local_effect
branch
from
c289a62 to
3642505
Compare
This PR adds the called function formatted string to the non-local-effect lint message.
This allows knowing which function triggered the lint if the issue is in a derived/expanded function (e.g., in a structure declaration).