refactor: including package path in implicit service name #13

ilharp · 2022-08-04T19:15:44Z

Thanks for this great work! I've used it in my open source project.

During use, I found that registering services with the same name will panic, even if they came from different packages. Here's a minimal repro:

// a/c/a-c.go
package c
import ( /* ... */ )
type MyStruct struct { /* ... */ }
func NewMyStruct(i *do.Injector) (*MyStruct, error) { /* ... */ }

// b/c/b-c.go
package c
import ( /* ... */ )
type MyStruct struct { /* ... */ }
func NewMyStruct(i *do.Injector) (*MyStruct, error) { /* ... */ }

// main.go
package main
import (
    ac "test/a/c"
    bc "test/b/c"
    "github.com/samber/do"
)
func main() {
    i := do.New()
    do.Provide(i, ac.NewMyStruct)
    do.Provide(i, bc.NewMyStruct)
}

// panic: DI: service `*c.MyStruct` has already been declared

The problem occurs because generateServiceName() didn't create a "global-unique" qualifier for type. This PR refactors generateServiceName() to create unique qualifiers by prepending package path.

Implementation

This implementation uses reflect.TypeOf(), which is the same as fmt.Sprintf("%T"). It basically the same as typeOfT.PkgPath() + "." + typeOfT.Name().

For the following code it generates such qualifiers:

// service_test.go
type testStruct struct{}
type testInterface interface{}

type	qualifier
`testStruct`	`github.com/samber/do.testStruct`
`*testStruct`	`github.com/samber/do.*testStruct`
`testInterface`	`github.com/samber/do.*testInterface`
`int`	`int`
`*int`	`*int`

Alternatives

`typeOfT.Name()` vs. `typeOfT.String()`

fmt.Sprintf("%T") uses typeOfT.String(). But typeOfT.String() says "The string representation may use shortened package names and is not guaranteed to be unique among types." Therefore it cannot be guaranteed for usage as a type qualifier.

Processing method of pointer indirect star

The current do.*test looks strange but I haven't thought of a better one yet.

Testing

Tests have all been updated. Maybe it's necessary to create two test packages to test bugs mentioned above, but adding two new files for this is somewhat weird so I didn't do that. Is it necessary?

Compatibility

This doesn't introduce API changes and therefore it should be possible to add into v1. But it introduces internal behavior changes so please consider it carefully.

During use, I found that registering services with the same name will panic, even if they came from different packages. Here's an minimal repro: ```go // a/c/a-c.go package c import ( /* ... */ ) type MyStruct struct { /* ... */ } func NewMyStruct(i *do.Injector) (*MyStruct, error) { /* ... */ } // b/c/b-c.go package c import ( /* ... */ ) type MyStruct struct { /* ... */ } func NewMyStruct(i *do.Injector) (*MyStruct, error) { /* ... */ } // main.go package main import ( /* ... */ ) func main() { i := do.New() do.Provide(i, ac.NewMyStruct) do.Provide(i, bc.NewMyStruct) } // panic: DI: service `*c.MyStruct` has already been declared ``` The problem occurs because `generateServiceName()` didn't create a "global-unique" qualifier for type. This PR refactors `generateServiceName()` to create unique qualifiers by prepending package path. \## Implementation This implementation uses `reflect.TypeOf()`, which is the same as `fmt.Sprintf("%T")`. It basically the same as `typeOfT.PkgPath() + "." + typeOfT.Name()`. For the following code it generates such qualifiers: ```go // service_test.go type testStruct struct{} type testInterface interface{} ``` type|qualifier -|- `testStruct`|`github.com/samber/do.testStruct` `*testStruct`|`github.com/samber/do.*testStruct` `testInterface`|`github.com/samber/do.*testInterface` `int`|`int` `*int`|`*int` \## Alternatives \### `typeOfT.Name()` vs. `typeOfT.String()` `fmt.Sprintf("%T")` uses `typeOfT.String()`. But `typeOfT.String()` [says](https://pkg.go.dev/reflect#Type) "The string representation **may** use shortened package names and is not guaranteed to be unique among types." Therefore it cannot be guaranteed for usage as a type qualifier. \### Processing method of pointer indirect star The current `do.*test` looks strange but I haven't thought of a better one yet. \## Testing Tests have all been updated. Maybe it's necessary to create two test packages to test bugs mentioned above, but adding two new files for this is somewhat weird so I didn't do that. Is it necessary? \## Compatibility This doesn't introduce API changes and therefore it should be possible to add into v1. But it introduces internal behavior changes so please consider it carefully.

samber

Thanks for this first contribution.

Do you know in what case typeOfT.String() might break?

I think we can create some packages under test/fixture/*/*.

samber · 2022-08-05T10:53:54Z

service.go

-	if name != "<nil>" {
-		return name
+	if typeOfT == nil {
+		typeOfT = reflect.TypeOf(new(T))


reflect.TypeOf(&t)

no need to allocate here ;)

ilharp · 2022-08-06T23:51:47Z

Downgrading PR to draft due to design defects.

I did some research and found some other problems of this PR.

`typeOfT.String()`

Do you know in what case typeOfT.String() might break?

Currently typeOfT.String() works well on all types using formats like package.TypeName (eg. do.Test). I didn't choose typeOfT.String() because the documentation clearly states that it is not guaranteed as type qualifers and may change at any time in the future. And there's another point that makes me drop typeOfT.String():

`typeOfT.Path() + typeOfT.String()`

Another consideration is of the format of typeOfT.String(). Consider *[]do.Test: it is actually a combination of extra indicator *[], package name do and type name Test. That means that if we use typeOfT.Path() + typeOfT.String(), the final result will be something like this:

    github.com/samber/do.    *[]    do    .Test
        |                     |      |      |
        |     Why indicator here?    |   Type name
        |                  Package name again?
  The package path (including package name)

To solve this problem and resort the final string, we have to add another piece of logic to split typeOfT.String() into parts. That of course hurts the robustness again.

Unnamed Types

Then I found another serious problem browsing the documentation: the typeOfT.PkgPath() and typeOfT.Name() return empty string for unnamed types. *do.Test and []do.Test are of course unnamed types. typeOfT.String() is not affected because it actually uses magic to get qualifiers "implemented in the runtime package", not info in reflect.Type.

This means that, if we don't handle all reflect.Kind (Chan, Func, Map, Pointer, etc.) manually, we won't able to use typeOfT.PkgPath() and obviously this PR will not achieve the goal.

For example a type like *map[string]*do.Test, we need to 1. extract typeOfT.Elem() to indirect pointer; 2. extract typeOfT.Key() and typeOfT.Elem() to get type of K/V; 3. extract typeOfT.Elem() again to finally get its base struct, and then get the package path to qualify it in the global. This is far more than such a lightweight DI can do and should do.

Conclusion

As a temporary solution, we can extract base struct only for maps (uses package path of value type), slices and pointers, then use typeOfT.Path() + typeOfT.String(). It may produce results like github.com/samber/do.*[]do.Test but will still have many edge cases, any of which can lead to bugs. I don't know if it's worth doing this any longer...

codecov-commenter · 2022-08-06T23:52:28Z

Codecov Report

Merging #13 (c5d56ed) into master (7123d38) will increase coverage by 0.44%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage   91.16%   91.60%   +0.44%     
==========================================
  Files           6        6              
  Lines         396      405       +9     
==========================================
+ Hits          361      371      +10     
+ Misses         26       25       -1     
  Partials        9        9

Flag	Coverage Δ
unittests	`91.60% <100.00%> (+0.44%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
service.go	`100.00% <100.00%> (+11.11%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

samber · 2023-09-24T17:20:19Z

Hi @ilharp 👋

I added a commit for supporting array and slice. Map, Func and Chan are still missing.

name = generateServiceName[*[]*[]**int]()
is.Equal("*[]*[]**int", name)

I will try to push it to do@v2 (with this code moved to a dedicated pkg eventually)

samber · 2023-09-24T20:54:21Z

I made a dedicated pkg with 95% type coverage -> https://github.com/samber/go-type-to-string

samber requested changes Aug 5, 2022

View reviewed changes

refactor: use &t

c5d56ed

ilharp marked this pull request as draft August 6, 2022 23:51

samber added 3 commits September 24, 2023 19:17

Service name: handle array+slice

477a12b

test(service name): handle array+slice

01131f3

oops

ac1dede

samber mentioned this pull request Oct 23, 2023

[👷] Road to v2 🚀 #45

Open

50 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: including package path in implicit service name #13

refactor: including package path in implicit service name #13

ilharp commented Aug 4, 2022 •

edited

samber left a comment •

edited

samber Aug 5, 2022

ilharp commented Aug 6, 2022

codecov-commenter commented Aug 6, 2022

samber commented Sep 24, 2023 •

edited

samber commented Sep 24, 2023

refactor: including package path in implicit service name #13

Are you sure you want to change the base?

refactor: including package path in implicit service name #13

Conversation

ilharp commented Aug 4, 2022 • edited

Implementation

Alternatives

typeOfT.Name() vs. typeOfT.String()

Processing method of pointer indirect star

Testing

Compatibility

samber left a comment • edited

Choose a reason for hiding this comment

samber Aug 5, 2022

Choose a reason for hiding this comment

ilharp commented Aug 6, 2022

typeOfT.String()

typeOfT.Path() + typeOfT.String()

Unnamed Types

Conclusion

codecov-commenter commented Aug 6, 2022

Codecov Report

samber commented Sep 24, 2023 • edited

samber commented Sep 24, 2023

ilharp commented Aug 4, 2022 •

edited

`typeOfT.Name()` vs. `typeOfT.String()`

samber left a comment •

edited

`typeOfT.String()`

`typeOfT.Path() + typeOfT.String()`

samber commented Sep 24, 2023 •

edited