Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: including package path in implicit service name #13

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ilharp
Copy link
Contributor

@ilharp ilharp commented Aug 4, 2022

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.
Copy link
Owner

@samber samber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/*/*.

service.go Outdated
if name != "<nil>" {
return name
if typeOfT == nil {
typeOfT = reflect.TypeOf(new(T))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reflect.TypeOf(&t)

no need to allocate here ;)

@ilharp ilharp marked this pull request as draft August 6, 2022 23:51
@ilharp
Copy link
Contributor Author

ilharp commented Aug 6, 2022

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
Copy link

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
Copy link
Owner

samber commented Sep 24, 2023

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
Copy link
Owner

samber commented Sep 24, 2023

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

@samber samber mentioned this pull request Oct 23, 2023
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants