Skip to content

Proposal: Add gopherjsvet tool #1219

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.

Sign up for GitHub

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

Jump to bottom
Open
3 of 11 tasks
flimzy opened this issue Jun 28, 2023 · 6 comments
Open
3 of 11 tasks

Proposal: Add gopherjsvet tool #1219

flimzy opened this issue Jun 28, 2023 · 6 comments
Milestone
Better developer ...

Comments

@flimzy
Copy link
Member

flimzy commented Jun 28, 2023 •
edited
Loading

Background

I just built a trivial linter during a recently 2-hour livestream on my YouTube channel. During my next livestream, I intend to start building a real/useful linter. And I had the idea of creating one for gopherjs.

So I'm opening this issue to get feedback and to solicit rule ideas to include.

Proposal

Create a new repo, github.com/gopherjs/gopherjsvet, which will contain a linter that can be embedded into golangci-lint, as well as compatible with go vet, and usable as a standalone binary.

Linter rules

My proposed linter rules (pending feasability, of course) so far are listed here. I'm soliciting suggestions for others. They should be issues that apply only to GopherJS code, as any other issues should go in a general-purpose linter. Most of these come from JavaScript Tips and Gotchas

  • Do not use items or fields of type js.Object directly
  • *js.Object must always be a pointer
  • A struct that embeds a *js.Object must always be a pointer.
  • Ensure that whenever js tags are present, a *js.Object field is embedded as the first struct field.
  • Ensure that struct literals for struct types that embed *js.Object do not also initialize other fields.
  • Ensure no non-scalar types in structs that embed *js.Object.
  • Complain about trying to json.Unmarshal into mixed Go/JS data structures.
  • Complain about blocking function calls in callbacks (not sure how easy this is actually to detect conclusively, but I'm sure we can get somewhere)
  • Ensure that proper build tags are set on any files that import github.com/gopherjs/gopherjs/js and syscall/js
  • Warn about use of the net/http package
  • (Optionally) warn about the use of encoding/json package when only unmarshaling.

Other Considerations

My immediate interest in this is mostly educational. So whether this proposal is accepted or not, I do intend to go forward (I just may do so in a private project, rather than one hosted under the gopherjs project).

Beyond that immediate goal, if this proves to be both feasable and of general interest, once the linter reaches a certain level of usefulness/maturity (criteria TBD), I would like to:

  • Publish pre-compiled binaries with goreleaser for common OSes (Linux, Mac, Windows probably)
  • Submit a PR to golangci-lint to include this linter
@nevkontakte
Copy link
Member

I like this idea, I think it would help avoiding a lot of possible pitfalls (aka know bugs :) ). I would also propose to integrate it into the gopherjs tool itself as a vet verb.

@flimzy
Copy link
Member Author

flimzy commented Jun 28, 2023

I would also propose to integrate it into the gopherjs tool itself as a vet verb

Ah yes, good (and obvoius) suggestion.

This makes me wonder, though... should the tool still live in a separate repo, or should it live here, where it can track with GopherJS releases more easily?

@nevkontakte
Copy link
Member

I think because it would also be a standalone tool, it makes most sense to keep it in a separate repository, but I could be convinced otherwise.

@flimzy
Copy link
Member Author

flimzy commented Jul 31, 2023 •
edited
Loading

I would also propose to integrate it into the gopherjs tool itself as a vet verb.

I attempted to work on this today, but it's not quite as easy as one might hope. The go/analysis/multichecker library is expected to be called directly from main(), and handles its own command line parsing. This gives us two options, as far as I can see:

  1. Simply call gopherjsvet as an external command. That means gopherjsvet has to be installed separately, which is a bit annoying.
  2. Copy-paste a bunch of code from golang.org/x/go/analysis/internal into GopherJS to tweak the behavior we need.

A third possibility, which I tried a bit, with little luck, would be to mangle os.Args before calling multichecker.Main. I'm not sure if this will give us all the flexibility we need, particularly with respect to build tags. I think we'll end up duplicating significant portions of the internal logic anyway. But maybe this approach would end up being less intrusive than option 2 above.

@nevkontakte
Copy link
Member

Might be worth asking in the gopher slack #tools channel, but I feel like option #1 is the only one really viable. It is really unfortunate that multichecker doesn't optionally accept something like flagset.

@flimzy
Copy link
Member Author

flimzy commented Aug 4, 2023 •
edited
Loading

So we have the same problem with option #1... gopherjsvet ignores the js and ecmascript build tags by default. Which means that I'll need to re-implement multichecker.Main anyway, unless we want to purely integrate with golangci-lint, which has its own init, naturally.

I'll work on a custom implementation, to see how ugly it gets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Better developer experience
Development

No branches or pull requests
2 participants
@nevkontakte @flimzy