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

feat: detect potential issues with struct pointers #36

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

Crocmagnon
Copy link
Owner

@Crocmagnon Crocmagnon commented Jan 13, 2025

Related to #34

@Crocmagnon Crocmagnon mentioned this pull request Jan 13, 2025
@Crocmagnon
Copy link
Owner Author

This is tricky. We should probably hide it behind a configuration of some sort (at least at first) because I have the feeling that it may trigger a lot.

return func(r *Container) {
ctx := r.Ctx
ctx = context.WithValue(ctx, "key", "val")
r.Ctx = ctx // want "nested context in function literal"
Copy link
Owner Author

Choose a reason for hiding this comment

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

This already triggers, without any of the linter changes in this PR.

func blah(r *Container) {
ctx := r.Ctx
ctx = context.WithValue(ctx, "key", "val")
r.Ctx = ctx // want "potential nested context in function declaration"
Copy link
Owner Author

@Crocmagnon Crocmagnon Jan 13, 2025

Choose a reason for hiding this comment

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

This is new.
Do we really want this? I feel like we're onto something here: this can definitely be the source of a fat context (see #34 (comment)), but it can also be pretty harmless, depending on how the function is called.

I'd be tempted to say we should at least signal it with a specific message, and let the user decide how they want to handle it.

@Crocmagnon
Copy link
Owner Author

Crocmagnon commented Jan 13, 2025

This is tricky. We should probably hide it behind a configuration of some sort (at least at first) because I have the feeling that it may trigger a lot.

Or can we rely on the fact that golangci-lint already provides a mechanism so that users can ignore specific linter rules?

@ldez do you have insights maybe? I wouldn't want to impose you with a surge of unwanted reports.
(I hope I'm not bothering you 🙏🏻)

@ldez
Copy link
Contributor

ldez commented Jan 14, 2025

It's better to have a linter option: by default, I expect 0 false-positives.

@Crocmagnon Crocmagnon force-pushed the feat/ignore-builtins branch 3 times, most recently from 326edc3 to 5030c3a Compare January 14, 2025 16:02
Base automatically changed from feat/ignore-builtins to master January 14, 2025 16:05
@Crocmagnon Crocmagnon force-pushed the feat/func-decl branch 2 times, most recently from dbcea46 to 3f63d5a Compare January 14, 2025 16:08
@Crocmagnon
Copy link
Owner Author

Thanks, noted 👍🏻

@Crocmagnon Crocmagnon force-pushed the feat/func-decl branch 2 times, most recently from bcd6581 to d01a98f Compare January 16, 2025 22:59

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@Crocmagnon Crocmagnon merged commit f887074 into master Jan 16, 2025
10 checks passed
@Crocmagnon Crocmagnon deleted the feat/func-decl branch January 16, 2025 23:03
@Crocmagnon Crocmagnon changed the title feat: detect potential nested contexts in function declarations feat: detect potential issues with struct pointers Jan 16, 2025
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.

None yet

2 participants