-
Notifications
You must be signed in to change notification settings - Fork 0
Added many, many features #2
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR introduces a sweeping refactor of NilAway’s error handling, diagnostic processing, and annotation inference across multiple components while enhancing documentation and build processes.
• Refactored error handling in /accumulation/analyzer.go to merge sub-analyzer errors (using errors.Join) and use a new diagnostic engine with clear inference mode switching.
• Updated README.md to include extensive integration guides and usage examples for standalone, golangci-lint, and bazel/nogo.
• Enhanced /Makefile with new testing targets and stricter golangci-lint version checks.
• Expanded configuration options in /config/config.go and /config/const.go to control pretty-printing and experimental features.
• Streamlined trigger and inference logic in annotation and inference modules for improved consistency and modularity.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
56 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: MethodResultReachesInterfacePrestring.String returns an empty string; verify if this behaviour is intended.
| } | |
| return false | |
| } | |
| func (m MethodResultReachesInterfacePrestring) String() string { | |
| return fmt.Sprintf("result %d of `%s` implementing `%s`", m.RetNum, m.ImplName, m.IntName) | |
| } |
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
|
|
||
| // As a last resort, convert the panics into errors and return. | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| e := fmt.Errorf("INTERNAL PANIC: %s\n%s", r, string(debug.Stack())) | ||
| funcChan <- functionResult{err: e, funcObj: funcObj} | ||
| } | ||
| }() | ||
|
|
||
| if contracts := inferContracts(fnssa); len(contracts) != 0 { | ||
| funcChan <- functionResult{ | ||
| funcObj: funcObj, | ||
| contracts: contracts, | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Goroutine closure captures loop variable 'funcObj'; pass it as a parameter to avoid race conditions.
| wg.Add(1) | |
| go func() { | |
| defer wg.Done() | |
| // As a last resort, convert the panics into errors and return. | |
| defer func() { | |
| if r := recover(); r != nil { | |
| e := fmt.Errorf("INTERNAL PANIC: %s\n%s", r, string(debug.Stack())) | |
| funcChan <- functionResult{err: e, funcObj: funcObj} | |
| } | |
| }() | |
| if contracts := inferContracts(fnssa); len(contracts) != 0 { | |
| funcChan <- functionResult{ | |
| funcObj: funcObj, | |
| contracts: contracts, | |
| } | |
| } | |
| }() | |
| wg.Add(1) | |
| go func(funcObj *types.Func, fnssa *ssa.Function) { | |
| defer wg.Done() | |
| // As a last resort, convert the panics into errors and return. | |
| defer func() { | |
| if r := recover(); r != nil { | |
| e := fmt.Errorf("INTERNAL PANIC: %s\n%s", r, string(debug.Stack())) | |
| funcChan <- functionResult{err: e, funcObj: funcObj} | |
| } | |
| }() | |
| if contracts := inferContracts(fnssa); len(contracts) != 0 { | |
| funcChan <- functionResult{ | |
| funcObj: funcObj, | |
| contracts: contracts, | |
| } | |
| } | |
| }(funcObj, fnssa) |
| writer := s2.NewWriter(&buf) | ||
| defer func() { | ||
| if cerr := writer.Close(); cerr != nil { | ||
| err = errors.Join(err, cerr) | ||
| } | ||
| }() | ||
|
|
||
| if err := gob.NewEncoder(writer).Encode(i.mapping); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Close the s2 writer before getting the bytes such that we have complete information. | ||
| if err := writer.Close(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Double call to writer.Close() in GobEncode. The deferred writer.Close() plus an explicit call may lead to redundant closing. Consider removing one of these calls.
| writer := s2.NewWriter(&buf) | |
| defer func() { | |
| if cerr := writer.Close(); cerr != nil { | |
| err = errors.Join(err, cerr) | |
| } | |
| }() | |
| if err := gob.NewEncoder(writer).Encode(i.mapping); err != nil { | |
| return nil, err | |
| } | |
| // Close the s2 writer before getting the bytes such that we have complete information. | |
| if err := writer.Close(); err != nil { | |
| return nil, err | |
| } | |
| writer := s2.NewWriter(&buf) | |
| if err := gob.NewEncoder(writer).Encode(i.mapping); err != nil { | |
| return nil, err | |
| } | |
| // Close the s2 writer before getting the bytes such that we have complete information. | |
| if err := writer.Close(); err != nil { | |
| return nil, err | |
| } |
| func (i *InferredMap) checkAnnotationKey(key annotation.Key) (annotation.Val, bool) { | ||
| shallowKey := newPrimitiveSite(key, false) | ||
| deepKey := newPrimitiveSite(key, true) | ||
| shallowKey := i.primitive.site(key, false) | ||
| deepKey := i.primitive.site(key, true) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: checkAnnotationKey depends on i.primitive; exported maps are created with nil primitive. Verify that calling CheckXXX on an exported fact does not lead to a nil dereference.
Note: I essentially made PR that added Nilaway's source code to any empty repo, effectively forcing Greptile to scan the repo. This is not exactly what Greptile is designed to do, so the results might be a bit strange.