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.

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

Add new linter: npecheck #3969

Closed
wants to merge 3 commits into from
Closed

Add new linter: npecheck #3969

wants to merge 3 commits into from

Conversation

chenfeining
Copy link

@chenfeining chenfeining commented Jul 22, 2023 •
edited
Loading

Check for potential nil pointer reference exceptions to compensate for go linter's shortcomings in this area. This linter is supposed to be the most rigorous and complete NPE solution.

1. linter repository

https://github.com/chenfeining/go-npecheck

2. description

  1. Focus on detecting potential nil pointer reference issues
  2. Support detecting potential nil pointer references for function ptr parameters and external function assignments. For example, inputPtr.v, assignedPtr.v, the inputPtr, assignedPtr can be nil pointers
  3. If the ancestor node is an external assignment pointer, it supports detecting chained variable references and whether there are potential nil pointer references. For example, p1.p2.v, the p1, p2 may be nil pointers
  4. If the parent node is an external assignment pointer, it supports detecting the problem of nil pointer reference of chained functions. For example, m.getFunc().v, both m and the m.GetFunc() may be a nil pointer
  5. In the case of slice including ptr elements from external assignment, support detection of slice ptr elements' potential nil pointer reference problem
  6. More details can see the linters' test case in go-npecheck testdata.

@chenfeining
Add new linter: npecheck
1e6c7d2 
Check potential null pointer reference exceptions, the most perfect check on the whole network.
@CLAassistant
Copy link

CLAassistant commented Jul 22, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@chenfeining chenfeining marked this pull request as draft July 22, 2023 13:28
@chenfeining chenfeining marked this pull request as ready for review July 22, 2023 13:40
@chenfeining chenfeining marked this pull request as draft July 22, 2023 13:46
@ldez
Copy link
Member

ldez commented Jul 22, 2023 •
edited
Loading

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

  • The CLA must be signed

Pull Request Description

  • It must have a link to the linter repository.
  • It must provide a short description of the linter.

Linter

  • It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
  • It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
  • The linter repository must have a CI and tests.
  • It must use go/analysis.
  • It must have a valid tag, ex: v1.0.0, v0.1.0.
  • It must not contain init().
  • It must not contain panic().
  • It must not contain log.fatal(), os.exit(), or similar.
  • It must not modify the AST.
  • It must not have false positives/negatives. (the team will help to verify that)
  • It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

  • They must have at least one std lib import.
  • They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

  • The linter must be added to the list of available linters (alphabetical case-insensitive order).
    • enable and disable options
  • If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
    • The values must be different from the default ones.
    • The default values must be defined in a comment.
    • The option must have a short description.

Others Requirements

  • The files (tests and linter) inside golangci-lint must have the same name as the linter.
  • The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
  • The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
  • The load mode (WithLoadMode(...)):
    • if the linter doesn't use types: goanalysis.LoadModeSyntax
    • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
  • The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.
  • WithURL() must contain the URL of the repository.

Recommendations

  • The linter should not use SSA. (SSA can be a problem with generics)
  • The linter repository should have a readme and linting.
  • The linter should be published as a binary. (useful to diagnose bug origins)

The golangci-lint team will edit this comment to check the boxes before and during the review.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

@ldez ldez added linter: new Support new linter feedback required Requires additional feedback labels Jul 22, 2023
@chenfeining
Optimize code
196e197 
1.  Use linter repository
2. Add test case
@chenfeining chenfeining marked this pull request as ready for review July 23, 2023 03:58
@chenfeining

This comment was marked as off-topic.

Sign in to view
@chenfeining

This comment was marked as off-topic.

Sign in to view
@ldez
Copy link
Member

ldez commented Jul 23, 2023 •
edited
Loading

The requirements defined in the checklist are still not met.

@ldez ldez added the blocked Need's direct action from maintainer label Jul 23, 2023
@ldez
Copy link
Member

ldez commented Jul 23, 2023 •
edited
Loading

There are many false positives (99,999999%).
The use of a pointer doesn't mean there is an NPE.
This linter is just a pointer detector.

By design, this linter can not meet our requirement for false positives.
I would recommend that you create a plugin.

@ldez ldez closed this Jul 23, 2023
@ldez ldez added declined and removed feedback required Requires additional feedback blocked Need's direct action from maintainer labels Jul 23, 2023
@chenfeining
Copy link
Author

chenfeining commented Jul 23, 2023 via email

@ldez
Copy link
Member

ldez commented Jul 23, 2023 •
edited
Loading

In fact, it should not be understood this way.

The problem is not how I understand this linter, but how you understand what is a linter.

This npecheck linter is also not enabled by default

This sentence is partially wrong because all the linters are enabled when using enable-all.

In fact, many people need this npecheck's linter function. Why not give that choice to the user?

Because your analyzer detects "potential" NPE. The majority of the reports are not NPE (false positives).
Your analyzer is not a linter but a pointer detector.
And there is already linter that detects real NPE.

Only a simple configuration is required, such as the following：

I think you have forgotten to add the configuration but it's not important because your analyzer is just offtopic.

@chenfeining
Copy link
Author

chenfeining commented Jul 23, 2023 via email

@chenfeining
Copy link
Author

chenfeining commented Jul 23, 2023 via email

@ldez
Copy link
Member

ldez commented Jul 23, 2023 •
edited
Loading

With enable-all, the project is dead.

I use enable-all on ALL my projects, so all my projects are dead?

enable-all is never a problem when your CI uses a fixed version of golangci-lint.

Why do you always consider most scenarios? Why aren't we allowed to take a small part of the situation.
And this small part of the situation may be the most central thing for some business scenarios.

I don't understand your sentences.

@chenfeining
Copy link
Author

chenfeining commented Jul 23, 2023 via email

@ldez
Copy link
Member

ldez commented Jul 23, 2023 •
edited
Loading

I think my explanations are clear: your analyzer is not an NPE linter but a pointer detector, which leads to a majority of useless reports (false positives).
Based on that your analyzer cannot be a part of golangci-lint.

Feel free to not agree but my decision is based on our organization's criteria defined by a team (and not just me as you suggest), they are the same criteria for all linters.

So now I will stop answering because this is a waste of time for you and me.

@chenfeining
Copy link
Author

chenfeining commented Jul 24, 2023 via email

@golangci golangci locked as off-topic and limited conversation to collaborators Jul 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
declined linter: new Support new linter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chenfeining @CLAassistant @ldez @zhangzhenhuachina