Report a hint for casting an expression of type T to same type T
#1072
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1072 +/- ##
==========================================
+ Coverage 77.28% 77.31% +0.02%
==========================================
Files 274 275 +1
Lines 35414 35492 +78
==========================================
+ Hits 27371 27439 +68
- Misses 6958 6964 +6
- Partials 1085 1089 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
SupunS
force-pushed
the
supun/warn-cast-2
branch
from
e6a8cfa to
a8c7784
Compare
| // - Case I: Contextually expected type is same as casted type (target type). | ||
| // - Case II: Expression is self typed, and is same as the casted type (target type). | ||
| func IsCastRedundant(expr ast.Expression, exprInferredType, targetType, expectedType Type) bool { | ||
| if expectedType != nil && expectedType.Equal(targetType) { |
There was a problem hiding this comment.
While Cadence doesn't currently have type aliases, we can probably future proof this against such a feature by changing this equal check to instead check whether both expectedType and targetType are subtypes of each other. At the moment this seems like it will only give the redundant cast hint when the two types are syntactically equal, but not when they are semantically equal.
There was a problem hiding this comment.
Hmm, that's a good point! But that might depend on how we define equality and casting for type aliases. Like for e.g: In Go, type aliases are 'equal' (no need to cast) at the user level, whereas typedefs are not (need to cast) (i.e: https://yourbasic.org/golang/type-alias/), and it would come down to how we are going to implement it for Cadence.
I totally agree that we need to be mindful of this when we do the aliasing, but wondering it might be overkill to do at this point, without knowing much about how aliasing going to work.
We use this equal method across the checker, and would need to update all such places when we introduce aliasing.
| // var y = x as Int8 // <-- not ok: `y` will be of type `Int8` with/without cast | ||
| // var y = x as Integer // <-- ok : `y` will be of type `Integer` | ||
| return exprType != nil && | ||
| exprType.Equal(targetType) |
| checkCastVisitor := &CheckCastVisitor{} | ||
|
|
||
| return checkCastVisitor.isCastRedundant(expr, exprInferredType, targetType) |
There was a problem hiding this comment.
Great idea to factor the casting check into its own type, though I think there is no need for a pointer type here (only the mutating functions need a pointer receiver):
| checkCastVisitor := &CheckCastVisitor{} | |
| return checkCastVisitor.isCastRedundant(expr, exprInferredType, targetType) | |
| return CheckCastVisitor{}.isCastRedundant(expr, exprInferredType, targetType) |
There was a problem hiding this comment.
CheckCastVisitor maintains an updates the exprInferredType and targetType as it goes into nested expressions (e.g: array/dictionary literals). So going to need a pointer receiver.
There was a problem hiding this comment.
Right, the mutating functions should have a pointer receiver, but the value here itself doesn't have to be, see e.g. https://go.dev/play/p/iF22kMUH_MV
There was a problem hiding this comment.
(d *CheckCastVisitor) IsRedundantCast() is actually the mutating function, and is recursively invoked by the others.
|
I know it is not totally related but below will give hint? Currently I guess the one below is not possible also. |
|
@bluesign |
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 2ea862a Results
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@turbolent It was added in 11e9ecc. It's turned off by default. |
|
@SupunS Ah, my bad, I had missed it 👍 |
| // WithLintingEnabled returns a checker option which enables/disables | ||
| // advanced linting. | ||
| // | ||
| func WithLintingEnabled(enabled bool) Option { |
There was a problem hiding this comment.
Must also be propagated to sub-interpreters in NewSubInterpreter
There was a problem hiding this comment.
This flag is for the checker. I think we don't need to pass it down?
Closes #1047
Description
This handles the Case-II in #1047: Expression is self-typed and is the same as casted type.
e.g:
masterbranchFiles changedin the Github PR explorer