refactor(depinject)!: require exported functions & types #12797

aaronc · 2022-08-02T09:39:52Z

Description

This PR:

adds the requirement that all provider and invoker functions and their parameters must be exported and not in internal packages so that these functions are available to codegen
makes ProviderDescriptor unexported so that custom providers (which aren't declared a compile time) cannot be built

It does not attempt to handle generic parameters because that involves some complex parsing. It can be added in a future PR if desired.

In a follow-up PR, we will need to make all existing module providers exported and change the core/appconfig code.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…ject-codegen-require-export

depinject/check_type.go

…ject-codegen-require-export

depinject/check_type.go

…-export' into aaronc/depinject-codegen-require-export

kocubinski · 2022-08-11T21:07:57Z

depinject/provider_desc.go

 	}

-	loc := LocationFromPC(val.Pointer())
+	loc := LocationFromPC(val.Pointer()).(*location)


Exported Providers and Invokers are a requirement for codegen, did you consider allowing them for them for runtime DI? Maybe this would be too confusing for users of this lib though.

I think you mean allowing unexported providers at runtime. I think it would be too confusing to have different behavior for runtime and compile time. Also, we want to ensure that all code can be executed at compile time if desired so doing this check at runtime ensures that.

kocubinski · 2022-08-11T21:14:51Z

Overall the direction makes sense given the constraints of codegen. It seems too complex to maintain side by side compatibility with the current runtime API, right?

Also I guess we can’t merge this without some refactoring (you mentioned this in the PR description). Is the plan to branch that work off of this PR?

aaronc · 2022-08-15T10:49:09Z

Overall the direction makes sense given the constraints of codegen. It seems too complex to maintain side by side compatibility with the current runtime API, right?

That's my thinking

Also I guess we can’t merge this without some refactoring (you mentioned this in the PR description). Is the plan to branch that work off of this PR?

No, this PR can be merged as is. The other code references a tag of depinject so we can do it in a totally separate PR either with a new tag or a replace directive

…ject-codegen-require-export

…cosmos/cosmos-sdk into aaronc/depinject-codegen-require-export

aaronc · 2022-08-17T12:44:41Z

depinject/provider_desc.go

-//	option.Provide(ProviderDescriptor{ ... })
-type ProviderDescriptor struct {
+//	option.Provide(providerDescriptor{ ... })
+type providerDescriptor struct {


Yes there was a replace directive but when I remove it go can't resolve the tagged module

…ject-codegen-require-export

julienrbrt · 2022-08-17T12:47:12Z

Don't we need to go work sync?

aaronc · 2022-08-17T12:47:48Z

Maybe the go.work file is inserting an automatic replace directive in the build?

julienrbrt · 2022-08-17T12:50:18Z

I believe we always use the latest version with go.work. I can check too.

aaronc · 2022-08-17T15:54:28Z

I'm checking to see if removing go.work fixes the issues here

…ject-codegen-require-export

codecov · 2022-08-30T17:44:22Z

Codecov Report

Merging #12797 (0c8e64e) into main (372a8f1) will increase coverage by 0.92%.
The diff coverage is 80.64%.

@@            Coverage Diff             @@
##             main   #12797      +/-   ##
==========================================
+ Coverage   53.17%   54.10%   +0.92%     
==========================================
  Files         641      662      +21     
  Lines       54871    56426    +1555     
==========================================
+ Hits        29180    30528    +1348     
- Misses      23365    23515     +150     
- Partials     2326     2383      +57

Impacted Files	Coverage Δ
depinject/module_dep.go	`92.30% <ø> (ø)`
depinject/simple.go	`100.00% <ø> (ø)`
depinject/provider_desc.go	`83.87% <74.57%> (ø)`
depinject/check_type.go	`76.31% <76.31%> (ø)`
depinject/config.go	`89.41% <100.00%> (ø)`
depinject/container.go	`89.85% <100.00%> (ø)`
depinject/struct_args.go	`98.41% <100.00%> (ø)`
x/group/keeper/keeper.go	`56.25% <0.00%> (-0.40%)`	⬇️
depinject/debug.go	`96.49% <0.00%> (ø)`
depinject/inject.go	`73.91% <0.00%> (ø)`
... and 18 more

depinject/config.go

kocubinski

LGTM, one nit on docstrings.

…-export' into aaronc/depinject-codegen-require-export

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

Close cosmos#11743 Since cosmos#12797, it's no longer possible to use non-exported functions as providers. The README used to contain examples with function literals as provider, so this change replace them with exported functions. An additional change updates the `BindInterface` arguments, which wasn't working on my side when running this code in go module called "duck".

Close cosmos#11743 Since cosmos#12797, it's no longer possible to use non-exported functions as providers. The README used to contain examples with function literals as provider, so this change replaces them with exported functions. An additional change updates the `BindInterface` arguments, which wasn't working on my side when running this code in go module called "duck".

Close cosmos#17743 Since cosmos#12797, it's no longer possible to use non-exported functions as providers. The README used to contain examples with function literals as provider, so this change replaces them with exported functions. An additional change updates the `BindInterface` arguments, which wasn't working on my side when running this code in go module called "duck".

aaronc added 9 commits August 2, 2022 11:39

refactor(depinject)!: require exported functions

e4b5846

unexport ProviderDescriptor

555c6fe

WIP on tests

88f0935

fix tests and check for bound instance methods

1f9f6c7

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/depin…

4fbf97a

…ject-codegen-require-export

address merge issues

b1f0c98

WIP on checking valid types

6e5ee43

WIP on checking valid types

9925907

WIP

5f2b439

github-advanced-security bot found potential problems Aug 10, 2022

View reviewed changes

depinject/check_type.go Fixed Show fixed Hide fixed

aaronc added 6 commits August 10, 2022 18:21

tests passing

30ffdb9

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/depin…

8db2564

…ject-codegen-require-export

revert changes outside module

ec9e610

docs

6546067

docs

75b4456

docs

0459519

aaronc marked this pull request as ready for review August 10, 2022 16:35

aaronc requested a review from a team as a code owner August 10, 2022 16:35

aaronc changed the title ~~refactor(depinject)!: require exported functions~~ refactor(depinject)!: require exported functions & types Aug 10, 2022

Merge branch 'main' into aaronc/depinject-codegen-require-export

0c0e801

kocubinski reviewed Aug 10, 2022

View reviewed changes

depinject/check_type.go Show resolved Hide resolved

aaronc and others added 3 commits August 11, 2022 12:17

add comment

6861c58

Merge remote-tracking branch 'origin/aaronc/depinject-codegen-require…

5975b98

…-export' into aaronc/depinject-codegen-require-export

Merge branch 'main' into aaronc/depinject-codegen-require-export

2788ed0

aaronc requested a review from kocubinski August 11, 2022 10:17

kocubinski reviewed Aug 11, 2022

View reviewed changes

aaronc added 2 commits August 15, 2022 12:51

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/depin…

227c7cf

…ject-codegen-require-export

Merge branch 'aaronc/depinject-codegen-require-export' of github.com:…

30c67e9

…cosmos/cosmos-sdk into aaronc/depinject-codegen-require-export

aaronc commented Aug 17, 2022

View reviewed changes

aaronc added 2 commits August 17, 2022 14:45

update depinject go.mod versions

8950c3f

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/depin…

c83d045

…ject-codegen-require-export

remove go.work

a5c4048

aaronc added 2 commits August 30, 2022 13:32

Merge branch 'main' of github.com:cosmos/cosmos-sdk into aaronc/depin…

811b316

…ject-codegen-require-export

add go.work back

d037fa0

julienrbrt mentioned this pull request Aug 30, 2022

go.work should be in .gitignore #13098

Closed

Merge branch 'main' into aaronc/depinject-codegen-require-export

ed1ef9f

julienrbrt approved these changes Aug 30, 2022

View reviewed changes

go mod tidy

b14fe7d

github-actions bot added the C:orm label Aug 30, 2022

Merge branch 'main' into aaronc/depinject-codegen-require-export

30d3205

kocubinski reviewed Aug 31, 2022

View reviewed changes

depinject/config.go Outdated Show resolved Hide resolved

kocubinski approved these changes Aug 31, 2022

View reviewed changes

aaronc added 2 commits August 31, 2022 13:20

fix docs

95a0df1

Merge remote-tracking branch 'origin/aaronc/depinject-codegen-require…

0c8e64e

…-export' into aaronc/depinject-codegen-require-export

aaronc enabled auto-merge (squash) August 31, 2022 17:21

aaronc merged commit 7728516 into main Aug 31, 2022

aaronc deleted the aaronc/depinject-codegen-require-export branch August 31, 2022 17:37

aaronc mentioned this pull request Aug 31, 2022

refactor: update core module to depinject changes #13115

Merged

19 tasks

tbruyelle mentioned this pull request Sep 15, 2023

docs(depinject): replace non-exported provider functions #17758

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(depinject)!: require exported functions & types #12797

refactor(depinject)!: require exported functions & types #12797

aaronc commented Aug 2, 2022 •

edited

Loading

kocubinski Aug 11, 2022

aaronc Aug 15, 2022

kocubinski commented Aug 11, 2022 •

edited

Loading

aaronc commented Aug 15, 2022

aaronc Aug 17, 2022

julienrbrt commented Aug 17, 2022 •

edited

Loading

aaronc commented Aug 17, 2022

julienrbrt commented Aug 17, 2022 •

edited

Loading

aaronc commented Aug 17, 2022

codecov bot commented Aug 30, 2022 •

edited

Loading

kocubinski left a comment •

edited

Loading

refactor(depinject)!: require exported functions & types #12797

refactor(depinject)!: require exported functions & types #12797

Conversation

aaronc commented Aug 2, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

kocubinski Aug 11, 2022

Choose a reason for hiding this comment

aaronc Aug 15, 2022

Choose a reason for hiding this comment

kocubinski commented Aug 11, 2022 • edited Loading

aaronc commented Aug 15, 2022

aaronc Aug 17, 2022

Choose a reason for hiding this comment

julienrbrt commented Aug 17, 2022 • edited Loading

aaronc commented Aug 17, 2022

julienrbrt commented Aug 17, 2022 • edited Loading

aaronc commented Aug 17, 2022

codecov bot commented Aug 30, 2022 • edited Loading

Codecov Report

kocubinski left a comment • edited Loading

Choose a reason for hiding this comment

aaronc commented Aug 2, 2022 •

edited

Loading

kocubinski commented Aug 11, 2022 •

edited

Loading

julienrbrt commented Aug 17, 2022 •

edited

Loading

julienrbrt commented Aug 17, 2022 •

edited

Loading

codecov bot commented Aug 30, 2022 •

edited

Loading

kocubinski left a comment •

edited

Loading