Introduce stringer #3594

RZhang05 · 2024-09-26T19:05:45Z

Working towards #3579

Description

A simple Stringer struct interface. It is intended in the future that all types which implement Stringer can be used in string templates.

Targeted PR against feature/string-templates branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work
Code follows the standards mentioned here
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

SupunS · 2024-10-01T22:06:20Z

@RZhang05 is this ready to be reviewed? or are there any pending changes?

RZhang05 · 2024-10-01T22:33:06Z

@RZhang05 is this ready to be reviewed? or are there any pending changes?

It is ready, note that there are some questions left in the code marked with TODO however.

runtime/sema/stringer.go

runtime/sema/type.go

runtime/tests/interpreter/stringer_test.go

turbolent

Nice work!

runtime/sema/type.go

turbolent · 2024-10-02T00:19:16Z

runtime/sema/type.go

+ // STRINGERTODO: other options? how to make existing simple types
+ // conform to an intersection type?


It isn't so much about conforming these types to an intersection type, but an interface, Stringer.

At the moment we don't have a means to specify a subtype relationship for simple types, so we might want to add that, i.e. generalize this solution a bit.

Note that the intersection type might contain multiple interfaces, e.g. might not just be {Stringer}, so checking the first element is not sufficient.

There's also no need, and it is not sufficient, to compare the names only, it is better to check for pointer equality here.

At the moment we don't have a means to specify a subtype relationship for simple types, so we might want to add that, i.e. generalize this solution a bit.

In 5f16333, I added the functionality for simple types to conform to an interface.

Not sure the best approach in general for all string convertible types to conform to Stringer since there are types such as TheAddressType and NumberType which are not SimpleTypes so I've left the edge case check in for those.

Added a ConformingType interface as suggested by #3594 (comment).

turbolent · 2024-10-02T00:30:28Z

runtime/sema/type.go

+ for len(interfaceTypes) > 0 {
+ lastIndex := len(interfaceTypes) - 1
+ interfaceType := interfaceTypes[lastIndex]
+ interfaceTypes[lastIndex] = nil
+ interfaceTypes = interfaceTypes[:lastIndex]
+
+ NativeInterfaceTypes[interfaceType.QualifiedIdentifier()] = interfaceType
+
+ nestedTypes := interfaceType.NestedTypes
+ if nestedTypes == nil {
+ continue
+ }
+
+ nestedTypes.Foreach(func(_ string, nestedType Type) {
+ nestedInterfaceType, ok := nestedType.(*InterfaceType)
+ if !ok {
+ return
+ }
+
+ interfaceTypes = append(interfaceTypes, nestedInterfaceType)
+ })
+ }


It might be good to refactor this into a function, and also use it in the init block initializing NativeCompositeTypes

turbolent · 2024-10-02T00:32:56Z

runtime/sema/stringer.go

+ "github.com/onflow/cadence/runtime/common"
+)
+
+const StringerTypeName = "Stringer"


Unfortunately we cannot define a single interface that users could implement for both structs and resources. To begin with we're only adding a struct interface, but we might also want to provide another resource interface in the future. We might want to consider naming this so it indicates that it is only for structs, e.g. StructStringer

turbolent

Great work!

turbolent · 2024-10-15T20:40:17Z

runtime/sema/gen/main.go

@@ -164,6 +164,9 @@ type typeDecl struct {
 memberDeclarations []ast.Declaration
 nestedTypes []*typeDecl
 hasConstructor bool
+
+ // used in simpleType generation
+ conformances []string


Maybe consider making this more type-safe by using a []common.TypeID instead

Changed to an InterfaceType for simplicity

turbolent · 2024-10-15T20:41:45Z

runtime/sema/gen/main.go

+ common.CompositeKindContract:
+ break
+ default:
+ panic(fmt.Sprintf("%s declarations are not supported", compositeKind.Name()))


Suggested change

panic(fmt.Sprintf("%s declarations are not supported", compositeKind.Name()))

panic(fmt.Sprintf("%s interface declarations are not supported", compositeKind.Name()))

turbolent · 2024-10-15T20:42:28Z

runtime/sema/gen/main.go

@@ -736,8 +741,151 @@ func (g *generator) VisitCompositeDeclaration(decl *ast.CompositeDeclaration) (_
 return
 }

-func (*generator) VisitInterfaceDeclaration(_ *ast.InterfaceDeclaration) struct{} {
- panic("interface declarations are not supported")
+func (g *generator) VisitInterfaceDeclaration(decl *ast.InterfaceDeclaration) (_ struct{}) {


Is there anything significantly different from VisitCompositeDeclaration? If the code generation is pretty much the same, maybe refactor it into a common function

turbolent · 2024-10-15T20:42:46Z

runtime/sema/gen/main.go

@@ -1591,6 +1739,9 @@ func simpleTypeLiteral(ty *typeDecl) dst.Expr {
 // Comparable: false,
 // Exportable: false,
 // Importable: false,
+ // comformances: []*InterfaceType {
+ // StructStringer,


Suggested change

// StructStringer,

// StructStringerType,

turbolent · 2024-10-15T20:44:25Z

runtime/sema/gen/main.go

@@ -2069,6 +2240,100 @@ func compositeTypeLiteral(ty *typeDecl) dst.Expr {
 }
 }

+func interfaceTypeExpr(ty *typeDecl) dst.Expr {


Same as above: Is there anything significantly different from the composite-related functions (compositeTypeExpr, compositeTypeLiteral)? If the code generation is pretty much the same, maybe refactor it into a common function

turbolent · 2024-10-15T20:51:22Z

runtime/sema/type.go

@@ -7244,11 +7276,18 @@ const AddressTypeName = "Address"

 // AddressType represents the address type
 type AddressType struct {


Same as above for NumericType

turbolent · 2024-10-15T20:54:21Z

runtime/sema/type.go

+ nestedCompositeType, ok := nestedType.(*CompositeType)
+ if !ok {
+ return
+ }
+
+ types = append(types, nestedCompositeType)


Add all nested types (e.g. interface types), not just composite types

Suggested change

nestedCompositeType, ok := nestedType.(*CompositeType)

if !ok {

return

}

types = append(types, nestedCompositeType)

types = append(types, nestedType)

turbolent · 2024-10-15T20:55:07Z

runtime/sema/type.go

+ nestedInterfaceType, ok := nestedType.(*InterfaceType)
+ if !ok {
+ return
+ }
+
+ types = append(types, nestedInterfaceType)


Same as above

Suggested change

nestedInterfaceType, ok := nestedType.(*InterfaceType)

if !ok {

return

}

types = append(types, nestedInterfaceType)

types = append(types, nestedType)

turbolent · 2024-10-15T20:55:51Z

runtime/sema/type.go

+ types = append(types, nestedInterfaceType)
+ })
+ }
+


Maybe add a default case and panic(errors.NewUnreachableError())

turbolent · 2024-10-15T20:57:27Z

runtime/sema/gen/main.go

@@ -736,8 +741,151 @@ func (g *generator) VisitCompositeDeclaration(decl *ast.CompositeDeclaration) (_
 return
 }

-func (*generator) VisitInterfaceDeclaration(_ *ast.InterfaceDeclaration) struct{} {
- panic("interface declarations are not supported")
+func (g *generator) VisitInterfaceDeclaration(decl *ast.InterfaceDeclaration) (_ struct{}) {


Please add a test case to sema/gen/testdata

… introduce-stringer

RZhang05 added 4 commits September 26, 2024 12:26

Initial test of stringer interface.

67d950b

Add test for custom toString.

6d60b9d

Add test for conformance error.

6298e1f

Fix tests for native interface types.

aa51455

SupunS self-assigned this Oct 1, 2024

RZhang05 marked this pull request as ready for review October 1, 2024 22:32

RZhang05 requested review from turbolent and SupunS as code owners October 1, 2024 22:32

SupunS reviewed Oct 2, 2024

View reviewed changes

turbolent reviewed Oct 2, 2024

View reviewed changes

RZhang05 changed the base branch from master to feature/string-templates October 2, 2024 15:17

RZhang05 added 5 commits October 2, 2024 15:18

Refactor stringer, improve generator, update simple types.

5f16333

Update tests.

ab157ad

Fix generator conditional conformances check.

72ff98c

Add ConformingType interface.

ad94aeb

Clean up unnecessary code.

26428cb

SupunS added the Feature label Oct 7, 2024

SupunS mentioned this pull request Oct 10, 2024

Supporting generating interface declarations in sema generator #2858

Open

Add simple tests, fix formatting.

c81e69c

turbolent reviewed Oct 15, 2024

View reviewed changes

Abstract common functionality, clean up.

fab5f6c

RZhang05 force-pushed the introduce-stringer branch from 1e12760 to fab5f6c Compare October 16, 2024 18:54

RZhang05 added 2 commits October 21, 2024 09:59

Merge remote-tracking branch 'upstream/feature/string-templates' into…

3d4f6a4

… introduce-stringer

Fix imports.

b9243e3

RZhang05 force-pushed the introduce-stringer branch from 49b69f8 to b9243e3 Compare October 21, 2024 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce stringer #3594

Introduce stringer #3594

RZhang05 commented Sep 26, 2024 •

edited

Loading

SupunS commented Oct 1, 2024

RZhang05 commented Oct 1, 2024

turbolent left a comment

turbolent Oct 2, 2024

RZhang05 Oct 2, 2024

RZhang05 Oct 3, 2024

turbolent Oct 2, 2024

turbolent Oct 2, 2024

turbolent left a comment

turbolent Oct 15, 2024

RZhang05 Oct 16, 2024

turbolent Oct 15, 2024

turbolent Oct 15, 2024

turbolent Oct 15, 2024

turbolent Oct 15, 2024

turbolent Oct 15, 2024

turbolent Oct 15, 2024

turbolent Oct 15, 2024

turbolent Oct 15, 2024

turbolent Oct 15, 2024

		// STRINGERTODO: other options? how to make existing simple types
		// conform to an intersection type?

	panic(fmt.Sprintf("%s declarations are not supported", compositeKind.Name()))
	panic(fmt.Sprintf("%s interface declarations are not supported", compositeKind.Name()))

		@@ -7244,11 +7276,18 @@ const AddressTypeName = "Address"

		// AddressType represents the address type
		type AddressType struct {

Introduce stringer #3594

Are you sure you want to change the base?

Introduce stringer #3594

Conversation

RZhang05 commented Sep 26, 2024 • edited Loading

Description

SupunS commented Oct 1, 2024

RZhang05 commented Oct 1, 2024

turbolent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbolent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RZhang05 commented Sep 26, 2024 •

edited

Loading