Ruby: RBI library changes to support models-as-data model generation #9932
Conversation
alexrford
added
no-change-note-required
alexrford
force-pushed
the
ruby/rbi-typegraph-fixes
branch
from
0cbe921 to
52305da
Compare
| /** | ||
| * Gets a fully qualified name for this constant. | ||
| */ | ||
| string getAQualifiedName() { none() } |
There was a problem hiding this comment.
The change looks fine; however getAQualifiedName is not used in this pull request so I wonder why you are changing this here.
There was a problem hiding this comment.
Ah, I was using this in an earlier revision of https://github.com/github/codeql-model-manager/pull/5 to get qualified names for general ConstantAccesses, but changed how this is implemented so this predicate isn't required. I'll drop it from this PR.
| } | ||
| // TODO: get associated method to which this can be passed | ||
| // TODO: widen type for procs/blocks | ||
| override MethodBase getAssociatedMethod() { none() } |
There was a problem hiding this comment.
Should this return Callable for procs/blocks/lambdas?
There was a problem hiding this comment.
We'd probably want this to return a BlockParameter, e.g. in:
sig do
params(
a: String,
block: T.nilable(T.proc.params(b: Integer).void)
).void
end
def blk_nilable_method(a, &block); end
the T.proc call corresponds to the &block parameter of blk_nilable_method, whereas the sig call corresponds to the blk_nilable_method definition itself. So we maybe need something like newtype TTypedCallable = TypedMethod(MethodBase m) or TypedBlock(BlockParameter b) or maybe to add another predicate BlockParameter getAssociatedBlockParameter() to the SignatureCall abstract class and in practice only one of getAssociatedMethod() and getAssociatedBlockParameter() will have a value.
| // explicitly exclude keyword parameters | ||
| not this.getAssociatedParameter(result.getName()) instanceof KeywordParameter and | ||
| // and exclude block arguments | ||
| not isBlockParamName(result.getName()) |
There was a problem hiding this comment.
Shouldn't this be not result instanceof BlockArgument instead? Or is using blk and block the way that RBI models block arguments and parameters?
There was a problem hiding this comment.
Good point - I think in this case we have something like:
sig do
params(
a: String,
block: T.nilable(T.proc.params(b: Integer).void)
).void
end
def blk_nilable_method(a, &block); end
so the argument here is block: T.nilable(T.proc.params(b: Integer).void) which is not a BlockArgument, but the associated parameter is &block so we can check if that is a BlockParameter instead which seems neater and less fragile.
These changes are mostly in service of better parameter type modelling - they should make more sense in the context of the models-as-data manager PR for a Ruby typegraph implementation.