Repository navigation
--report-glean fix: Name methods on Literal and LiteralString receivers by their class - #5605
Quaden2307 wants to merge 3 commits into
Conversation
|
✅ This PR meets all requirements from our pull request checklist. Optional recommendations:
Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. |
|
This pull request has been imported. If you are a Meta employee, you can view this in D131090557. (Because this pull request was imported automatically, there will not be any future comments.) |
rchen152
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left one readability suggestion. Once that's addressed, this should be good to merge.
| let defining_type = |ty: Type| { | ||
| // A literal is an instance of its general class (e.g. `"a"` is a `str`), | ||
| // so its members are named after that class. | ||
| let ty = match ty { |
There was a problem hiding this comment.
nit: Splitting the logic between two matchs makes it sort of hard to read. I'd recommend something like:
let cls = match &ty {
... // extract the `ClassType`
_ => return ty,
};
// get the class member, falling back to `Type::ClassType(cls.clone())`There was a problem hiding this comment.
Done, thanks for the feedback!
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
rchen152
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
Thanks again, @Quaden2307! |
|
This pull request has been merged in 5619b5e. |
Summary
--report-gleanwould not properly name a method on a Literal or LiteralString receiver. For example,"hello".title()would emittitlerather than the expectedstr.title. This is because the closure that finds a method's defining class only handled class types, so aLiteralfell through without a class name. The fix was at the start ofdefining_typeclosure, aType::Literalis converted to its class withgeneral_class_type, and aType::LiteralStringtostr. The existingClassTypearm after that handles the naming.Fixes #5250
Test Plan
Added a
string_literalcase to the gleansnapshot_test, with the five receivers from the issue. The snapshot was regenerated and all five methods now show asstr.title/str.join.cargo test snapshot_testpasses, and none of the other snapshots changed.test.pylint also passes.