-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
refactor(depinject)!: require exported functions & types (#12797)
* refactor(depinject)!: require exported functions * unexport ProviderDescriptor * WIP on tests * fix tests and check for bound instance methods * address merge issues * WIP on checking valid types * WIP on checking valid types * WIP * tests passing * revert changes outside module * docs * docs * docs * add comment * revert * update depinject go.mod versions * remove go.work * add go.work back * go mod tidy * fix docs Co-authored-by: Julien Robert <julien@rbrt.fr>
- Loading branch information
1 parent
372a8f1
commit 7728516
Showing
16 changed files
with
525 additions
and
283 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| package depinject | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "strings" | ||
| "unicode" | ||
|
|
||
| "github.com/pkg/errors" | ||
| "golang.org/x/exp/slices" | ||
| ) | ||
|
|
||
| // isExportedType checks if the type is exported and not in an internal | ||
| // package. NOTE: generic type parameters are not checked because this | ||
| // would involve complex parsing of type names (there is no reflect API for | ||
| // generic type parameters). Parsing of these parameters should be possible | ||
| // if someone chooses to do it in the future, but care should be taken to | ||
| // be exhaustive and cover all cases like pointers, map's, chan's, etc. which | ||
| // means you actually need a real parser and not just a regex. | ||
| func isExportedType(typ reflect.Type) error { | ||
| name := typ.Name() | ||
| pkgPath := typ.PkgPath() | ||
| if name != "" && pkgPath != "" { | ||
| if unicode.IsLower([]rune(name)[0]) { | ||
| return errors.Errorf("type must be exported: %s", typ) | ||
| } | ||
|
|
||
| pkgParts := strings.Split(pkgPath, "/") | ||
| if slices.Contains(pkgParts, "internal") { | ||
| return errors.Errorf("type must not come from an internal package: %s", typ) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| switch typ.Kind() { | ||
| case reflect.Array, reflect.Slice, reflect.Chan, reflect.Pointer: | ||
| return isExportedType(typ.Elem()) | ||
|
|
||
| case reflect.Func: | ||
| numIn := typ.NumIn() | ||
| for i := 0; i < numIn; i++ { | ||
| err := isExportedType(typ.In(i)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| numOut := typ.NumOut() | ||
| for i := 0; i < numOut; i++ { | ||
| err := isExportedType(typ.Out(i)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
|
|
||
| case reflect.Map: | ||
| err := isExportedType(typ.Key()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return isExportedType(typ.Elem()) | ||
|
|
||
| default: | ||
| // all the remaining types are builtin, non-composite types (like integers), so they are fine to use | ||
| return nil | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package depinject | ||
|
|
||
| import ( | ||
| "os" | ||
| "reflect" | ||
| "testing" | ||
|
|
||
| "gotest.tools/v3/assert" | ||
|
|
||
| "cosmossdk.io/depinject/internal/graphviz" | ||
| ) | ||
|
|
||
| func TestCheckIsExportedType(t *testing.T) { | ||
| expectValidType(t, false) | ||
| expectValidType(t, uint(0)) | ||
| expectValidType(t, uint8(0)) | ||
| expectValidType(t, uint16(0)) | ||
| expectValidType(t, uint32(0)) | ||
| expectValidType(t, uint64(0)) | ||
| expectValidType(t, int(0)) | ||
| expectValidType(t, int8(0)) | ||
| expectValidType(t, int16(0)) | ||
| expectValidType(t, int32(0)) | ||
| expectValidType(t, int64(0)) | ||
| expectValidType(t, float32(0)) | ||
| expectValidType(t, float64(0)) | ||
| expectValidType(t, complex64(0)) | ||
| expectValidType(t, complex128(0)) | ||
| expectValidType(t, os.FileMode(0)) | ||
| expectValidType(t, [1]int{0}) | ||
| expectValidType(t, []int{}) | ||
| expectValidType(t, "") | ||
| expectValidType(t, make(chan int)) | ||
| expectValidType(t, make(<-chan int)) | ||
| expectValidType(t, make(chan<- int)) | ||
| expectValidType(t, func(int, string) (bool, error) { return false, nil }) | ||
| expectValidType(t, func(int, ...string) (bool, error) { return false, nil }) | ||
| expectValidType(t, In{}) | ||
| expectValidType(t, map[string]In{}) | ||
| expectValidType(t, &In{}) | ||
| expectValidType(t, uintptr(0)) | ||
| expectValidType(t, (*Location)(nil)) | ||
|
|
||
| expectInvalidType(t, container{}, "must be exported") | ||
| expectInvalidType(t, &container{}, "must be exported") | ||
| expectInvalidType(t, graphviz.Attributes{}, "internal") | ||
| expectInvalidType(t, map[string]graphviz.Attributes{}, "internal") | ||
| expectInvalidType(t, []graphviz.Attributes{}, "internal") | ||
| } | ||
|
|
||
| func expectValidType(t *testing.T, v interface{}) { | ||
| t.Helper() | ||
| assert.NilError(t, isExportedType(reflect.TypeOf(v))) | ||
| } | ||
|
|
||
| func expectInvalidType(t *testing.T, v interface{}, errContains string) { | ||
| t.Helper() | ||
| assert.ErrorContains(t, isExportedType(reflect.TypeOf(v)), errContains) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
7728516There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting a failure on latest main, which is caused by this commit:
make clean build && ./build/simdoutputs:
Any idea?
7728516There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I cannot reproduce
Any more information you can add about your environment?
7728516There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this was fixed on main