better error and dca for configurables #6121
Conversation
Benchmark for a3a48dbClick to view benchmark
|
Benchmark for 6b6ef74Click to view benchmark
|
Benchmark for 24f90f9Click to view benchmark
|
sway-error/src/error.rs
Outdated
| ), | ||
| hints: vec![], | ||
| help: vec
There was a problem hiding this comment.
Hmm, having just this first sentence in the help without the second proposed one that suggests to verify if the function is actually there, this help actually becomes confusing because it states that the function is defined in the "code::codec" and the error is there very likely because it is not defined.
There is another one issue. The error is currently emitted on every configurable because the span points to an individual configurable. It should be just a single error pointing to either the whole configurable block or to the text configurable, because it is not specific to a particular configurable.
Together with the above help message it results in actually a confusing experience.
Coming to an optimally crafted diagnostic is never easy 😄 and usually takes some polishing. After looking once again at the overall formulation I propose the following changes.
We leave the old-style single line formulation in line 967 as it is.
Reason should answer why is this an issue. The fact that "Configurables need a function named "abi_decode_in_place" to be in scope" is not an issue that's how it should be 😄
So, to be consistent with the guidelines, proposal is to reformulate the Reason as: Function "abi_decode_in_place" is not in scope
Issue::error should actually point at the configurable block or even better at the span of the "configurable" and further explain: Configurables need a function named "abi_decode_in_place" to be in scope. // With the fullstop this time 😄
Note that we can still emit the error for every configurable. That's fine because the deduplication will remove duplicates and leave only a single error message.
And finally in the help we have both sentences, to guide where to look:
"Function "abi_decode_in_place" is usually defined in the standard library module "core::codec".".into(),
"Verify that you are using a version of the "core" standard library that contains this function.".into(),
Note also the added word "usually" because we allow developers to define their own encoding/decoding mechanisms by implementing functions in scope.
There was a problem hiding this comment.
There is another one issue. The error is currently emitted on every configurable because the span points to an individual configurable. It should be just a single error pointing to either the whole configurable block or to the text configurable, because it is not specific to a particular configurable.
Not sure I understood this point. What is not being used is the configurable, not the block. Why highlighting the whole block would be better?
There was a problem hiding this comment.
The warning is perfectly fine, of course 😄 I was talking about the error which is not related to a particular configurable and will be rendered on every configurable. Currently for e.g.:
configurable {
MAX_SUPPLY: u64 = 3,
OTHER: u64 = 3,
MORE: u64 = 3,
}rendered errors will be quite overwhelming and confusing:
The impression those message give is, that there is something problematic with an individual configurable, but they are all perfectly fine and there is nothing to change about them. It is actually a piece of "infrastructure" that is missing. That's why I think having a single error (duplicates automatically removed via deduplication) pointing to the block will better communicate the issue.
There was a problem hiding this comment.
Ah. In this case it makes sense.
There was a problem hiding this comment.
I will use the keyword span, as the whole block renders very strangely.
There was a problem hiding this comment.
Yes unfortunately this issue with code snippets is a know one, #5499, but still open. Using keyword is also fine.
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Igor Rončević <[email protected]>
Co-authored-by: Igor Rončević <[email protected]>
|
As discussed, the error now points to the configurable block:
|
xunilrj
force-pushed
the
xunilrj/configurables-better-error-and-dca
branch
from
58152c6
to
0a6fdb2
Compare
Benchmark for 56c2e27Click to view benchmark
|
Benchmark for 408d703Click to view benchmark
|
Description
This PR closes #6118.
If for some reason an older
coreends up being used, we now warn with a more user-friendly error.We also use a better span when warning about unused configurables.
Checklist
Breaking*orNew Featurelabels where relevant.