[Checker] check and reject composite types with non-storable fields #180
Conversation
turbolent
added
the
Language Breaking Change
| // check if all members' type are allowed to be the fields | ||
| for _, member := range members { | ||
| // if a member is a non-storable field, then report error | ||
| if !member.IsStorable() { | ||
| err := &FieldTypeNotStorableError{ | ||
| Name: member.Identifier.Identifier, | ||
| Type: member.TypeAnnotation.Type, | ||
| Pos: member.Identifier.Pos, | ||
| } | ||
| checker.report(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring this into a function
| t.Run("check error message", func(t *testing.T) { | ||
| testcase := cases["function is not a storable field"] | ||
| _, err := ParseAndCheck(t, testcase.code) | ||
| require.Error(t, err) | ||
|
|
||
| checkerError, _ := err.(*sema.CheckerError) | ||
| require.Equal(t, "field fn is not storable, type: ((): Int)", | ||
| checkerError.ChildErrors()[0].Error()) | ||
| }) |
There was a problem hiding this comment.
We don't check error messages. I'd remove this.
If you want to keep it, like above, use errs := ExpectCheckerErrors(t, err, 1) and then errs[0]. This properly checks if the error occurred and the correct number of child errors exist.
In the current code checkerError can be nil, so ChildErrors() will crash.
If it is non-nil, ChildErrors() could also return an empty slice, so the indexing ([0]) will crash
| // print the failed the cadence code if test case was broken | ||
| require.NoError(t, err, errmsg) | ||
| } else { | ||
| require.Error(t, err, errmsg) |
There was a problem hiding this comment.
Check the actual errors here, not just that something went wrong: The checker could reject the tests for other reasons.
The common pattern is:
errs := ExpectCheckerErrors(t, err, 1)
assert.IsType(t, &sema.FieldTypeNotStorableError{}, errs[0])
ExpectCheckerErrors ensures the error is an error and has the correct number of child errors
Co-authored-by: Bastian Müller <bastian@axiomzen.co>
…into leo/3931-storable-fields
Working towards #62 #169
Description
For contributor use:
masterbranchFiles changedin the Github PR explorer