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

Upgrade the Go version to v1.18, and x/tools to v0.1.11. Support generics better. #325

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

guodongli-google
Copy link
Contributor

@guodongli-google guodongli-google commented May 6, 2022

Address the differences related to the new SSA format.
Remove errors/warnings on generics types, and add unit tests.

Fixes #323

It's a good idea to open an issue first for discussion.

  • Running against a large codebase such as Kubernetes does not error out. (See DEVELOPING.md for instructions on how to do that.)
  • Appropriate changes to README are included in PR

Address the differences related to the new SSA format.
Remove errors/warnings on generics types, and add unit tests.
Address the differences related to the new SSA format.
Remove errors/warnings on generics types, and add unit tests.
@guodongli-google guodongli-google changed the title Upgrade the Go version to v1.18, and x/tools to v0.1.11. Fix unit test errors due to Go versions. May 6, 2022
Address the differences related to the new SSA format.
Remove errors/warnings on generics types, and add unit tests.
@guodongli-google
Copy link
Contributor Author

Have no idea why the linter "addlicense" fails since it succeeds locally.

@guodongli-google guodongli-google changed the title Fix unit test errors due to Go versions. Upgrade the Go version to v1.18, and x/tools to v0.1.11. May 6, 2022
@guodongli-google guodongli-google changed the title Upgrade the Go version to v1.18, and x/tools to v0.1.11. Upgrade the Go version to v1.18, and x/tools to v0.1.11. Support generics better. May 6, 2022
Copy link
Collaborator

@PurelyApplied PurelyApplied left a comment

Choose a reason for hiding this comment

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

I think we should signal to the user somewhere that generic support is not enabled. That could be in the README inclusive-or as a message when these skip blocks are hit. But it seems like a gap in coverage that a user might be expecting otherwise.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
go.mod Outdated
sigs.k8s.io/yaml v1.2.0
)

// TODO: clean up indirect imports.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this actionable? I thought // indirect imports were the result of a direct import not building with a go.mod that details explicit dependencies, so the consumer has to choose explicit versions. But other than removing our direct dependencies, I don't think we get to clean up the indirect ones, do we?

I think having them grouped separately is cleaned up enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These indirect imports are actually produced automatically by go mod tidy, and I don't have a better way to handle them now.

internal/pkg/sourcetype/sourcetype.go Outdated Show resolved Hide resolved
@guodongli-google
Copy link
Contributor Author

I think we should signal to the user somewhere that generic support is not enabled. That could be in the README inclusive-or as a message when these skip blocks are hit. But it seems like a gap in coverage that a user might be expecting otherwise.

Agreed. However warning each generics usage will be too much, and it is better to specify it somewhere else, e.g. in a README file.
There are follow-up CLs where I want to handle the generics better, e.g. through the "InstantiateGenerics" mode. Nevertheless, the full support won't be available soon. I'd let you recommend how to put in such a message.

Copy link
Collaborator

@PurelyApplied PurelyApplied left a comment

Choose a reason for hiding this comment

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

I'm sorry to have let this get so stale.

I saw Tim's #327 and have started a thread there, but to summarize: it would be nice to fold this and that PR into each other. I can go either way on either requiring 1.18+ vs Tim's init() approach to set the expectation between interface{} vs any, but like this PR's addition of tests (which I suppose require 1.18).

@sjbarag
Copy link

sjbarag commented Feb 28, 2024

Looks like most of this landed via #327. Are there still generics-support gaps?

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.

Generics are not supported by analyzers
3 participants