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

Error when types are imported but seemingly unused in Go #3399

Closed
1 of 5 tasks
echeung-amzn opened this issue Feb 25, 2022 · 5 comments · Fixed by #3664
Closed
1 of 5 tasks

Error when types are imported but seemingly unused in Go #3399

echeung-amzn opened this issue Feb 25, 2022 · 5 comments · Fixed by #3664
Assignees
Labels
bug This issue is a bug. language/go Regarding GoLang bindings p1

Comments

@echeung-amzn
Copy link

🐛 Bug Report

Affected Languages

  • TypeScript or Javascript
  • Python
  • Java
  • .NET (C#, F#, ...)
  • Go

General Information

  • JSII Version: 1.54.0
  • Platform: jsii/superchain:1-buster-slim image

What is the problem?

This is in relation to https://github.com/cdklabs/cdk-monitoring-constructs

Given some code that looks like:

import * as elasticsearch from "monocdk/aws-elasticsearch";
import * as opensearch from "monocdk/aws-opensearchservice";

export type Domain =
  | opensearch.CfnDomain
  | opensearch.IDomain
  | elasticsearch.CfnDomain
  | elasticsearch.IDomain;

Go build fails with the following:

Error: #STDERR> ./cdkmonitoringconstructs.go:21:2: imported and not used: "github.com/aws/aws-cdk-go/awscdk/awselasticsearch"
Error: #STDERR> ./cdkmonitoringconstructs.go:23:2: imported and not used: "github.com/aws/aws-cdk-go/awscdk/awsopensearchservice"

(See failed build)

We also attempted importing the type only, i.e.:

import type {
  CfnDomain as ElasticSearchCfnDomain,
  IDomain as ElasticSearchIDomain,
} from "monocdk/aws-elasticsearch";
import type {
  CfnDomain as OpenSearchCfnDomain,
  IDomain as OpenSearchIDomain,
} from "monocdk/aws-opensearchservice";

export type Domain =
  | ElasticSearchCfnDomain
  | ElasticSearchIDomain
  | OpenSearchCfnDomain
  | OpenSearchIDomain;

(See failed build)

I also attempted to reduce the codebase down to just the above for a minimum repo, but ended up with errors like:

[2022-02-25T12:03:14.385] [ERROR] jsii/compiler - Type model errors prevented the JSII assembly from being created
error JSII9002: Unable to resolve type "monocdk.IDomain". It may be @internal or not exported from the module's entry point (as configured in "package.json" as "main").
lib/index.ts:15:15 - error JSII3002: Type "monocdk.IDomain" cannot be used as a parameter type because it is not exported from monocdk

15   constructor(domain: Domain) {
                 ~~~~~~

  node_modules/monocdk/lib/aws-opensearchservice/lib/domain.d.ts:548:1
    548 export interface IDomain extends cdk.IResource {
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    549     /**
        ~~~~~~~
    ... 
    756     metricIndexingLatency(props?: MetricOptions): Metric;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    757 }
        ~
    The referenced type is declared here

Verbose Log

I unfortunately can't seem to do a build locally due to network policies. Let me know if I can somehow provide more info otherwise.

@echeung-amzn echeung-amzn added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 25, 2022
@ryparker ryparker added the p1 label Feb 25, 2022
@NGL321 NGL321 added language/go Regarding GoLang bindings and removed needs-triage This issue or PR still needs to be triaged. labels May 25, 2022
@polothy
Copy link
Contributor

polothy commented Jul 18, 2022

Ran into this same problem. Just another example of it:

import * as rds from "aws-cdk-lib/aws-rds";

export interface MyProps {
  readonly database: rds.ServerlessCluster | rds.DatabaseCluster | rds.DatabaseInstance;
}

It compiles to this in Go:

import (
	"github.com/aws/aws-cdk-go/awscdk/v2/awsrds"
)

type MyProps struct {
	Database interface{} `field:"required" json:"database" yaml:"database"`
}

And the awsrds import goes unused.

@RomainMuller RomainMuller self-assigned this Jul 18, 2022
@RomainMuller
Copy link
Contributor

RomainMuller commented Jul 18, 2022

Yeah seems like a code generator bug around type unions, as those end up being represented as interface{} for lack of a union support in go. We need to detect this case and suppress the import when that is the only reference there is.

A workaround would be to actually expose a type from the library outside of the context of a type union... Obviously this means exposing an undesired API... so it's not ideal.

I'm going to look into addressing this in the next couple of days.... hopefully we can release a fix soon.

@polothy
Copy link
Contributor

polothy commented Jul 18, 2022

Awesome, thanks! Looks like goimports could also cleanup imports. It's installed separately though.

RomainMuller added a commit that referenced this issue Jul 19, 2022
When imported types are solely referenced by type unions, a go import
is emitted, but is never used (type unions end up represented as opaque
`interface{}` type). This causes compilation failures.

Added a test case for this scenario in particular, and adjusted go code
generation to not emit dependency imports for type unions.

These imports may be re-introduced soon, as we are working to add
dynamic type checking around type unions in go (at which point those
imports would no longer be unused).

Fixes #3399
@RomainMuller
Copy link
Contributor

RomainMuller commented Jul 19, 2022

@polothy yeah I'm aware of goimports but I'd rather avoid adding dependencies on additional tooling as much as possible. We ought to be able to generate "clean" go code out of the box, instead of having to post-process that.

I've pushed out a PR with a fix for the particular issue experienced here. The fix turned out to be trivial.

As noted on the PR comments, we are working to add dynamic/runtime type checking around type union locations in the go code (#3641), which is going to make those imports be actually useful in the future... Hopefully soon 🙃

RomainMuller added a commit that referenced this issue Jul 21, 2022
When imported types are solely referenced by type unions, a go import
is emitted, but is never used (type unions end up represented as opaque
`interface{}` type). This causes compilation failures.

Added a test case for this scenario in particular, and adjusted go code
generation to not emit dependency imports for type unions.

These imports may be re-introduced soon, as we are working to add
dynamic type checking around type unions in go (at which point those
imports would no longer be unused).

Fixes #3399
@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. language/go Regarding GoLang bindings p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants