-
Notifications
You must be signed in to change notification settings - Fork 1
Make squin identity sites an argument rather than an attribute #480
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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
if not isinstance(sites, ir.ResultValue): | ||
return (interp.lattice.top(),) | ||
|
||
if not isinstance(site_stmt := sites.stmt, py.Constant): | ||
return (interp.lattice.top(),) | ||
|
||
if not isinstance(value := site_stmt.value, ir.PyAttr): | ||
return (interp.lattice.top(),) |
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.
this should return bottom instead of top tho
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.
also this does not deal with alias, so double check if the analysis are run before alias inline pass
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.
or, assuming wrap const and cont prop is done before this analysis, then you can get the constant it from hint["const"]
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.
Yeah, it's not run, which is why there were no hints. But I'll just include it in the run then.
this should return bottom instead of top tho
Can you explain why? I thought since there is no error, but you just can't know how many sites there are since sites
is not a constant that top would be the way to go as it's AnySites
.
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.
ah right... nvm
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.
just change the get constant as @weinbe58 mentioned below then
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'm good so long as you implement the method table using the hints, if there is no hint return unknown
if you want an example of how to use the hints look at the implementation here:
|
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.
Just some minor comments
@kaihsin pinging for another round of review. I think we should be good to go! |
Closes #479 .
As expected, this makes the implementation of the
NSitesAnalysis
a bit more tricky and also leads to situations where you can't infer the number of sites of an identity.