-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: Update PoemHandlerParam to use getCanonicalPath #19801
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
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.
Pull Request Overview
Updates the PoemHandlerParam predicate to use getCanonicalPath on an inferred type rather than the deprecated getResolvedPath.
- Added imports for
TypeInferenceandType - Switched from matching
getResolvedPathto inferring the type and checking its canonical path - Refactored the
existsclause to bind the inferred type and perform the new path comparison
Comments suppressed due to low confidence (3)
rust/ql/lib/codeql/rust/frameworks/Poem.qll:16
- [nitpick] The variable name
tis ambiguous; consider renaming it toparamTypeorinferredTypefor clarity.
exists(TupleStructPat param, Type t |
rust/ql/lib/codeql/rust/frameworks/Poem.qll:12
- The doc comment still refers to handler parameters in terms of path resolution; update it to mention canonical path checking via type inference.
* Parameters of a handler function
rust/ql/lib/codeql/rust/frameworks/Poem.qll:16
- Add unit tests for
PoemHandlerParamto cover bothQueryandPathcases with the new canonical path logic.
exists(TupleStructPat param, Type t |
|
Consistency check failures accepted. DCA is clean. |
|
Consistency check failures accepted again. |
| @@ -1,29 +1,44 @@ | |||
| multipleMethodCallTargets | |||
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.
I think you should just revert this file to get green CI.
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.
Trying it now. Not really sure what's going on with them.
|
All concerns addressed, CI is green, DCA was clean. I'm away tomorrow so if you approve please please hit merge as well, then I hopefully don't need to come back on Monday to a fresh merge conflict. 🤞 |
|
Thanks! |
Update
PoemHandlerParamto usegetCanonicalPathrather thangetResolvedPath.@hvitved