Changed everything across the repo #1
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request overhauls NilAway by systematically refactoring its error handling, annotation processing, inference, and test infrastructure. The changes aim to improve robustness, consistency, and clarity for both internal diagnostics and end-user reporting.
• accumulation/analyzer.go now uses errors.Join and a new diagnostic engine to combine results from sub-analyzers via analysishelper.Result.
• Annotation files (affiliation, consume_trigger, full_trigger, key, produce_trigger) have been refactored to consistently use pointers for deep copying and equality, with careful updates to trigger creation and comparison.
• The README and Makefile were updated with clearer instructions, enhanced module support, and stricter linting targets.
• Inference and assertion packages now use an ordered map and new primitivizer logic to deterministically propagate nilability, while several legacy files (e.g. nilcheck.go, trusted_func.go) were removed.
• Test suites have been consolidated, enhanced for panic recovery, cancellation handling, and goroutine leak detection.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
56 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| [nilness analyzer](https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/nilness), however, it employs much more | ||
| sophisticated and powerful static analysis techniques to track nil flows within a package as well _across_ packages, and | ||
| report errors providing users with the nilness flows for easier debugging. | ||
| report errors providing users with the nilness flows for easier debugging. |
There was a problem hiding this comment.
style: Sentence on line 13 feels a bit awkward: consider rephrasing for clarity (e.g. 'and reports errors by displaying the nilness flow for easier debugging').
| NilAway, in its current form, can report false positives. This unfortunately hinders its immediate | ||
| merging in [golangci-lint][golangci-lint] and be offered as a linter (see [PR#4045][pr-4045]). | ||
| Therefore, you need to build NilAway as a plugin to golangci-lint to be executed as a private | ||
| linter. There are two plugin systems in golangci-lint, and it is much easier to use the |
There was a problem hiding this comment.
style: The phrase 'merging in golangci-lint and be offered as a linter' may be grammatically improved. Consider rewording for clarity.
| res := <-resultChan | ||
| require.Equal(t, res.index, 0) | ||
| require.ErrorIs(t, res.err, context.Canceled) |
There was a problem hiding this comment.
style: Consider revising require.Equal parameter ordering to (expected, actual) for clearer error messages.
| path, producers := r.ParseExprAsProducer(consumer.Expr, false) | ||
| if path == nil { // expr is not trackable | ||
| if producers == nil { | ||
| return // expr is not trackable, but cannot be nil, so do nothing | ||
| // Here we can infer that the expression is non-nil by definition. Instead of ignoring creation of a trigger, | ||
| // particularly for always safe tracking, we create a trigger with ProduceTriggerNever. | ||
| if c, ok := consumer.Annotation.(*annotation.UseAsReturn); ok && c.IsTrackingAlwaysSafe { | ||
| r.AddNewTriggers(annotation.FullTrigger{ | ||
| Producer: &annotation.ProduceTrigger{ | ||
| Annotation: &annotation.ProduceTriggerNever{}, | ||
| Expr: consumer.Expr, | ||
| }, | ||
| Consumer: consumer, | ||
| }) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
logic: In AddConsumption, when path is nil and producers is nil, a ProduceTriggerNever trigger is created for always safe tracking. Confirm that this branch correctly identifies non-trackable yet non-nil expressions.
There was a problem hiding this comment.
logic: File completely removed. Verify that the new design covers the contract parsing responsibilities and that no dependent module is left without proper function contract extraction.
| p := pass.Fset.File(d.Pos).Name() | ||
| for _, e := range excludes { | ||
| if strings.HasPrefix(p, e) { | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: File name (pass.Fset.File(d.Pos).Name()) may be relative while the parsed prefixes are absolute; ensure paths are normalized before prefix matching.
| p := pass.Fset.File(d.Pos).Name() | |
| for _, e := range excludes { | |
| if strings.HasPrefix(p, e) { | |
| return | |
| } | |
| } | |
| p := pass.Fset.File(d.Pos).Name() | |
| absPath, err := filepath.Abs(p) | |
| if err == nil { | |
| p = absPath | |
| } | |
| for _, e := range excludes { | |
| if strings.HasPrefix(p, e) { | |
| return | |
| } | |
| } |
No description provided.