Skip to content

Add a lint for struct constructors #2708

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

Closed
elauffenburger opened this issue Mar 31, 2022 · 7 comments
Closed

Add a lint for struct constructors #2708

elauffenburger opened this issue Mar 31, 2022 · 7 comments
Labels
enhancement New feature or improvement linter: idea idea of a linter

Comments

@elauffenburger
Copy link

Your feature request related to a problem? Please describe.

I've run into runtime bugs when adding a new field to a "service-like" struct (a FooService or something like that), but then I find out someone's instantiating the struct without a constructor so some code somewhere breaks at runtime because of a nil pointer!

Describe the solution you'd like.

I'd love to see two linters created:

  • structs that should have a ctor, which don't have a ctor
  • structs that do have a ctor, but which are instantiated without using the ctor

These linters would definitely want to take regexes that specify the types of structs that we consider to be lintable -- most structs don't have this problem since they're just data and we really don't even want a ctor for them.

Describe alternatives you've considered.

Pray our tests catch this stuff! 🙏

Additional context.

I've written these linters already (and they're useful for me), but I didn't want to create a PR without gauging interest first.

I'm new to this project, so thanks for your patience!

@elauffenburger elauffenburger added the enhancement New feature or improvement label Mar 31, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 31, 2022

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

@ldez
Copy link
Member

ldez commented Mar 31, 2022

Hello,

your linters seem really specific to your use case, I'm not sure there is a big demand for this type of linter.

My recommendations:

  • open source your projects
  • create 1 linter instead 2
  • allow your linter to run as a plugin
  • allow your linter to run as a binary

@elauffenburger
Copy link
Author

elauffenburger commented Mar 31, 2022

Okay! Yep, make sense -- could I elaborate on this a bit more? Just to make it a bit more clear (because I do think this is a somewhat general pattern and tl;dr is for DI)!

This is totally contrived, but:

package metrics

struct Service {
  cfg     Config
  client  Client
}

func NewService(cfg Config, client Client) Service {
  return Service { cfg, client }
}

func SomethingSilly() {
  // All good here; this is just some data, so no need to use a ctor or anything.
  cfg := Config{}

  // Wait! I bypassed the ctor, so now I don't have the client set!
  svc := Service { cfg: cfg }
}

// ...

package foo

func DoTheThing() {
  // Wait! This definitely won't work!
  metricsSvc := metrics.Service{}
}

Does that illustrate this any better?

@ldez
Copy link
Member

ldez commented Mar 31, 2022

I understand your use case, but your linter will only be useful if all the structs follow the same name pattern.
If the structs don't follow a strict pattern, the configuration will be complex or will create a lot of false-positive/false-negative.

So I'm still thinking that is a specific use case.

But I don't represent the whole community, so we will see, with this issue, if there is some interest for this linter.

@ldez
Copy link
Member

ldez commented Mar 31, 2022

Also without the code, I cannot see what is the approach you implemented in your linter, and it's difficult to judge.

So I think that my recommendations can help:

  • open source your projects
  • create 1 linter instead 2
  • allow your linter to run as a plugin
  • allow your linter to run as a binary

@elauffenburger
Copy link
Author

Okay! Nope, that makes sense -- thanks for taking a look. Feel free to close this if you feel that makes sense!

@alingse

This comment was marked as off-topic.

@ldez ldez added the linter: idea idea of a linter label Aug 28, 2022
@ldez ldez closed this as completed Aug 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement linter: idea idea of a linter
Projects
None yet
Development

No branches or pull requests

3 participants