From 6fe8b6e09e54ea9b201852a68d29925b5c482216 Mon Sep 17 00:00:00 2001 From: Grant Nelson Date: Fri, 27 Sep 2024 10:12:01 -0600 Subject: [PATCH] Update for integration tests --- compiler/compiler_test.go | 29 ++++++------ compiler/internal/dce/README.md | 40 ++++++++++++----- compiler/internal/dce/dce_test.go | 75 +------------------------------ compiler/internal/dce/info.go | 19 ++------ 4 files changed, 46 insertions(+), 117 deletions(-) diff --git a/compiler/compiler_test.go b/compiler/compiler_test.go index c17037e94..9c831d456 100644 --- a/compiler/compiler_test.go +++ b/compiler/compiler_test.go @@ -204,9 +204,7 @@ func TestDeclSelection_RemoveUnusedFuncInstance(t *testing.T) { sel.DeclCode.IsDead(`^\s*Foo = function`) sel.DeclCode.IsDead(`^\s*sliceType(\$\d+)? = \$sliceType\(\$Int\)`) - - // TODO(gn): This should not be alive because it is not used. - sel.DeclCode.IsAlive(`^\s*Sum\[\d+ /\* int \*/\]`) + sel.DeclCode.IsDead(`^\s*Sum\[\d+ /\* int \*/\]`) } func TestDeclSelection_RemoveUnusedStructTypeInstances(t *testing.T) { @@ -229,9 +227,8 @@ func TestDeclSelection_RemoveUnusedStructTypeInstances(t *testing.T) { sel.DeclCode.IsAlive(`^\s*Foo\[\d+ /\* int \*/\] = \$newType`) sel.DeclCode.IsAlive(`^\s*\$ptrType\(Foo\[\d+ /\* int \*/\]\)\.prototype\.Bar`) - // TODO(gn): This should not be alive because it is not used. - sel.DeclCode.IsAlive(`^\s*Foo\[\d+ /\* float64 \*/\] = \$newType`) - sel.DeclCode.IsAlive(`^\s*\$ptrType\(Foo\[\d+ /\* float64 \*/\]\)\.prototype\.Bar`) + sel.DeclCode.IsDead(`^\s*Foo\[\d+ /\* float64 \*/\] = \$newType`) + sel.DeclCode.IsDead(`^\s*\$ptrType\(Foo\[\d+ /\* float64 \*/\]\)\.prototype\.Bar`) } func TestDeclSelection_RemoveUnusedInterfaceTypeInstances(t *testing.T) { @@ -262,11 +259,12 @@ func TestDeclSelection_RemoveUnusedInterfaceTypeInstances(t *testing.T) { sel.InitCode.IsDead(`\$pkg\.F64 = FooBar\[\d+ /\* float64 \*/\]`) sel.DeclCode.IsAlive(`^\s*FooBar\[\d+ /\* int \*/\]`) - // TODO(gn): Below should be alive because it is an arg to FooBar[int]. + // The Foo[int] instance is defined as a parameter in FooBar[int] that is alive. + // However, Foo[int] isn't used directly in the code so it can be removed. + // JS will simply duck-type the Baz object to Foo[int] without Foo[int] specifically defined. sel.DeclCode.IsDead(`^\s*Foo\[\d+ /\* int \*/\] = \$newType`) - // TODO(gn): Below should be dead because it is only used by a dead init. - sel.DeclCode.IsAlive(`^\s*FooBar\[\d+ /\* float64 \*/\]`) + sel.DeclCode.IsDead(`^\s*FooBar\[\d+ /\* float64 \*/\]`) sel.DeclCode.IsDead(`^\s*Foo\[\d+ /\* float64 \*/\] = \$newType`) } @@ -295,9 +293,7 @@ func TestDeclSelection_RemoveUnusedMethodWithDifferentSignature(t *testing.T) { sel.DeclCode.IsAlive(`^\s*Foo = \$newType`) sel.DeclCode.IsAlive(`\s*\$ptrType\(Foo\)\.prototype\.Bar`) - // TODO(gn): Below should be dead because it is not used even though - // its name matches a used unexported method. - sel.DeclCode.IsAlive(`\s*\$ptrType\(Foo\)\.prototype\.baz`) + sel.DeclCode.IsDead(`\s*\$ptrType\(Foo\)\.prototype\.baz`) sel.DeclCode.IsAlive(`^\s*Foo2 = \$newType`) sel.DeclCode.IsAlive(`\s*\$ptrType\(Foo2\)\.prototype\.Bar`) @@ -334,10 +330,11 @@ func TestDeclSelection_RemoveUnusedUnexportedMethodInstance(t *testing.T) { sel.DeclCode.IsAlive(`^\s*Foo\[\d+ /\* uint \*/\] = \$newType`) sel.DeclCode.IsAlive(`\s*\$ptrType\(Foo\[\d+ /\* uint \*/\]\)\.prototype\.Bar`) - // TODO(gn): All three below should be dead because Foo[uint].baz is unused. - sel.DeclCode.IsAlive(`\s*\$ptrType\(Foo\[\d+ /\* uint \*/\]\)\.prototype\.baz`) - sel.DeclCode.IsAlive(`^\s*Baz\[\d+ /\* uint \*/\] = \$newType`) - sel.DeclCode.IsAlive(`\s*\$ptrType\(Baz\[\d+ /\* uint \*/\]\)\.prototype\.Bar`) + + // All three below are dead because Foo[uint].baz is unused. + sel.DeclCode.IsDead(`\s*\$ptrType\(Foo\[\d+ /\* uint \*/\]\)\.prototype\.baz`) + sel.DeclCode.IsDead(`^\s*Baz\[\d+ /\* uint \*/\] = \$newType`) + sel.DeclCode.IsDead(`\s*\$ptrType\(Baz\[\d+ /\* uint \*/\]\)\.prototype\.Bar`) } func TestDeclSelection_RemoveUnusedTypeConstraint(t *testing.T) { diff --git a/compiler/internal/dce/README.md b/compiler/internal/dce/README.md index 4fe2882c0..db6408d62 100644 --- a/compiler/internal/dce/README.md +++ b/compiler/internal/dce/README.md @@ -35,9 +35,10 @@ may be safely eliminated, i.e. not outputted to the JS file(s). The following is the logic behind the DCE mechanism. Not all of the following is used since some conditions are difficult to determine even with a lot of -additional information. To ensure that the JS output is fully functional, -we bias the DCE towards things being alive. We'd rather keep something we -don't need than remove something that is needed. +additional information, and because GopherJS stores some additional information +making some parts of DCE unnecessary. To ensure that the JS output is fully +functional, we bias the DCE towards things being alive. We'd rather keep +something we don't need than remove something that is needed. ### Package @@ -92,10 +93,14 @@ All the types in the function signatures and embedded interfaces are the dependents of the interface. Interfaces may contain exported and unexported function signatures. -If an interface is alive then all of the functions, even the unexported -functions, are alive. +If an interface is alive then all of the functions are alive. Since there are many ways to wrap a type with an interface, any alive type that -duck-types to an interface must have all of the matching methods alive. +duck-types to an interface must have all of the matching methods also alive. + +In theory the unexported functions are also alive however, for GopherJS there +is an exception because duck-typing is handled separately the method +definitions. Those difference are discussed in [Dependencies](#dependencies) +but for this idea we discuss DCE more generally. Since the exported methods in an alive type will be alive, see [Named Types](#named-types), the only ones here that need to be considered @@ -247,6 +252,13 @@ the unexported method alive. Since the unexported method is only visible in the package in which it is defined, the package path is included in the method name. +To simplify the above for GopherJS, we don't look at the receiver for +an unexported method before indicating it is alive. Meaning if there is no +interface, only two named objects with identical unexported methods, the use +of either will indicate a use of both. This will cause slightly more unexported +method to be alive while reducing the complication of type checking which object +or type of object is performing the call. + | Declaration | exported | unexported | non-generic | generic | object name | method name | |:------------|:--------:|:----------:|:-----------:|:-------:|:------------|:------------| | variables | █ | █ | █ | n/a | `.` | | @@ -324,18 +336,22 @@ is recursive, e.g. `Foo[Bar[Bar[...]]]`. ### Dependencies -The dependencies are initialized via two paths. - -The first is dependencies that are specified in an expression. +The dependencies that are specified in an expression. For example a function that invokes another function will be dependent on that invoked function. When a dependency is added it will be added as one or more names to the declaration that depends on it. It follows the [naming rules](#naming) so that the dependencies will match correctly. -The second is structural dependencies that are specified automatically while -the declaration is being named. When an interface is named, it will -automatically add all unexported signatures as dependencies via +In theory, structural dependencies would be needed to be added +automatically while the declaration is being named. When an interface is named, +it would automatically add all unexported signatures as dependencies via `.()()`. +However, we do not need to do that in GopherJS because we aren't using +the existence of realized methods in duck-typing. GopherJS stores full set +of method information when describing the type so that even when things like +unexported methods in interfaces are removed, duck-typing will still work +correctly. This reduces the size of the code by not keeping a potentially +long method body when the signature is all that is needed. Currently we don't filter unused packages so there is no need to automatically add dependencies on the packages themselves. This is also why the package diff --git a/compiler/internal/dce/dce_test.go b/compiler/internal/dce/dce_test.go index 226e90c7e..da4f54466 100644 --- a/compiler/internal/dce/dce_test.go +++ b/compiler/internal/dce/dce_test.go @@ -248,26 +248,6 @@ func Test_Info_SetNameAndDep(t *testing.T) { objectFilter: `jim.wembley`, }, }, - { - name: `interface with unexported methods setting dependencies`, - obj: parseObject(t, `Hoggle`, - `package jim - type Hoggle interface{ - cowardly() bool - loyalTo(goblin string) bool - makePrinceOfTheBogOfEternalStench() error - }`), - want: Info{ - objectFilter: `jim.Hoggle`, - // The automatically defined dependencies for unexported methods - // in the interface that match with the methodFilter of unexported methods. - deps: map[string]struct{}{ - `jim.cowardly() bool`: {}, - `jim.loyalTo(string) bool`: {}, - `jim.makePrinceOfTheBogOfEternalStench() error`: {}, - }, - }, - }, { name: `unexported method resulting in an interface with exported methods`, obj: parseObject(t, `bear`, @@ -439,54 +419,6 @@ func Test_Info_SetNameAndDep(t *testing.T) { methodFilter: `jim.sidekick() jim.Beaker[any]`, }, }, - { - name: `unexported method in interface from named embedded interface`, - obj: parseObject(t, `Waldorf`, - `package jim - type Statler interface{ - boo() - } - type Waldorf interface{ - Statler - }`), - want: Info{ - objectFilter: `jim.Waldorf`, - deps: map[string]struct{}{ - `jim.boo()`: {}, - }, - }, - }, - { - name: `unexported method in interface from unnamed embedded interface`, - obj: parseObject(t, `Waldorf`, - `package jim - type Waldorf interface{ - interface{ - boo() - } - }`), - want: Info{ - objectFilter: `jim.Waldorf`, - deps: map[string]struct{}{ - `jim.boo()`: {}, - }, - }, - }, - { - name: `unexported method on instance of generic interface`, - obj: parseObject(t, `Waldorf`, - `package jim - type Statler[T any] interface{ - boo() T - } - type Waldorf Statler[string]`), - want: Info{ - objectFilter: `jim.Waldorf`, - deps: map[string]struct{}{ - `jim.boo() string`: {}, - }, - }, - }, { name: `struct with self referencing type parameter constraints`, obj: parseObject(t, `Keys`, @@ -503,7 +435,7 @@ func Test_Info_SetNameAndDep(t *testing.T) { }, }, { - name: `struct with self referencing type parameter constraints`, + name: `interface with self referencing type parameter constraints`, obj: parseObject(t, `ElectricMayhem`, `package jim type ElectricMayhem[K comparable, V any, M ~map[K]V] interface { @@ -513,11 +445,6 @@ func Test_Info_SetNameAndDep(t *testing.T) { }`), want: Info{ objectFilter: `jim.ElectricMayhem[comparable, any, ~map[comparable]any]`, - deps: map[string]struct{}{ - `jim.keys() []comparable`: {}, - `jim.values() []any`: {}, - `jim.asMap() ~map[comparable]any`: {}, - }, }, }, { diff --git a/compiler/internal/dce/info.go b/compiler/internal/dce/info.go index e9a63baf5..244da8f17 100644 --- a/compiler/internal/dce/info.go +++ b/compiler/internal/dce/info.go @@ -22,14 +22,14 @@ type Info struct { objectFilter string // methodFilter is the secondary DCE name for a declaration. - // This will be empty if objectFilter is empty. + // This usually will be empty if objectFilter is empty. // This will be set to a qualified method name if the objectFilter // can not determine if the declaration is alive on it's own. // See ./README.md for more information. methodFilter string - // List of fully qualified (including package path) DCE symbol identifiers the - // symbol depends on for dead code elimination purposes. + // Set of fully qualified (including package path) DCE symbol + // and/or method names that this DCE declaration depends on. deps map[string]struct{} } @@ -55,7 +55,7 @@ func (d *Info) String() string { // unnamed returns true if SetName has not been called for this declaration. // This indicates that the DCE is not initialized. func (d *Info) unnamed() bool { - return d.objectFilter == `` + return d.objectFilter == `` && d.methodFilter == `` } // isAlive returns true if the declaration is marked as alive. @@ -87,17 +87,6 @@ func (d *Info) SetName(o types.Object, tArgs ...types.Type) { // Determine name(s) for DCE. d.objectFilter, d.methodFilter = getFilters(o, tArgs) - - // Add automatic dependencies for unexported methods on interfaces. - if n, ok := o.Type().(*types.Named); ok { - if it, ok := n.Underlying().(*types.Interface); ok { - for i := it.NumMethods() - 1; i >= 0; i-- { - if m := it.Method(i); !m.Exported() { - d.addDepName(getMethodFilter(m, tArgs)) - } - } - } - } } // addDep add a declaration dependencies used by DCE