Ruby: assume some global constants are defined#10928
Conversation
| not mid instanceof ModuleBase and | ||
| not mid instanceof Block | ||
| ) | ||
| private ModuleBase enclosingModuleNoBlock(Stmt node) { |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter
| * | ||
| * Includes `scope` itself and the final module/block. | ||
| */ | ||
| private Scope enclosingScopesNoBlock(Scope scope) { |
There was a problem hiding this comment.
Why is it important to stop at block scopes? I'm not sure this change is right. For example:
A = "top"
module M
A = "M"
end
M.module_eval { puts A }
prints "top"
There was a problem hiding this comment.
You're right. I'm not sure what I was thinking there. The block thing is specific to include calls and such since they rely on self.
Technically there was a bug in old implementation of enclosingModule since it relies on getParent(), although it would only affect really obscure cases like,
module (include A)::B
endwhere the include call would be seen as being part of the B module, not the top-level. Still, it's a footgun that might reappear later so I think it's best to keep the new formulation.
I've pushed a commit to fix the place where we incorrectly relied on this predicate.
When classes are missing a canonical name, we end up not resolving its outgoing subclass edges. For example, we missed that
Unkn::Bis a subclass ofUnkn::Ain this example:Here is an example of a class that was affected by this issue.
This PR doesn't try to fix the problem in general, but rather aims to pick some low-hanging fruit, enough to unblock my work on migrating some models from AST to DataFlow.
We simply treat a constant
Cas being defined at the top-level if we've seenCreferenced in a context where it can only refer to a top-level constant.Interestingly, the API graph does contain a subclassing edge in the above example, as this isn't dependent on the classes having canonical names. API graphs of course has the luxury of not being in the same SCC as constant resolution itself, but it would be nice if we could make the two systems agree on the superclass/ancestor relationship. But that's for another PR.
I noticed some call graph changes due to the
Stringclass now being considered resolved, and this affects flow through pattern-matching in a few cases. I decided to addStringtobuiltins()to ensure this is consistently the case, though I'm not sure if it's technically a built-in or just a class in the standard library (does it matter?).Evaluation looks good
opalseems to be due to the call toconst_getnow resolved to a definition so no longer anUnknownMethodCall.