Ruby: split standard library models into multiple files#7886
Conversation
dbc3112 to
55ca8e0
Compare
alexrford
left a comment
There was a problem hiding this comment.
This is generally really nice and makes things much neater.
I'm conflicted on the frameworks -> libraries rename. It seems more accurate as to what we're actually modelling, but JS and Python generally bundle up both frameworks and libraries under the frameworks umbrella term. I also have some fears about breaking external queries and libraries that currently import QLLs that are moved or deleted in this PR.
On balance I'd personally prefer to keep these under frameworks for consistency with other languages.
For similar reasons of avoiding breaking external queries, we might want to keep a version of frameworks/StandardLibrary.qll around that re-exports public modules/classes/predicates of Core.qll and Stdlib.qll as deprecated versions. Ideally we would just be able to mark StandardLibrary.qll itself as deprecated, but I'm not aware of a simple way to do this.
| import core.BasicObject::BasicObject | ||
| import core.Object::Object | ||
| import core.Kernel::Kernel | ||
| import core.Module | ||
| import core.Array | ||
| import core.Regexp |
There was a problem hiding this comment.
What's the criteria for importing the module within a QLL (e.g. core.Kernel::Kernel) vs. the QLL itself (e.g. core.Module)?
There was a problem hiding this comment.
The reason I did this was to reduce churn in other files that previously imported StandardLibrary.qll and then used the classes directly. I can add a followup commit that explicitly imports the required modules in those files, and then change these imports back to import core.Object; import core.Kernel. Maybe that would be better as a followup, to avoid piling too many things in this PR? What do you think?
There was a problem hiding this comment.
Having this as a followup sounds good to me.
|
I guess this is going to clash with #7878, but I'm not sure how soon that PR is going to be in a mergeable state. |
|
I added the |
002286d to
b125fc0
Compare
892a9c4 to
89d2879
Compare
FWIW don't let the existence of that PR slow you down. I'm confident I can resolve conflicts efficiently on my end, and I'd be happy to help resolve conflicts in other PRs once mine is merged. |
|
If I understood correctly, sticking with the |
|
OK, let's go with frameworks. |
Split the classes modeling various standard library concepts into a structured group of multiple files. Things that are part of the core language live in framworks/core and standard libraries (that aren't part of core) live in frameworks/stdlib. This mirrors the structure followed by the Ruby docs (https://docs.ruby-lang.org/en/3.1/). Tests are split in a followup commit.
codeql.ruby.frameworks.StandardLibrary is deprecated
This file re-exports everything it used to define, marking each as deprecated to warn users that they should import `Core` or `Stdlib` instead.
89d2879 to
bfd2c14
Compare
alexrford
left a comment
There was a problem hiding this comment.
The test output needs to be regenerated, otherwise LGTM.
|
Ugh that's so annoying! I thought this was ready to go... |
Split the classes modeling various standard library concepts into a
structured group of multiple files.
Things that are part of the core language live in libraries/core and
standard libraries (that aren't part of core) live in libraries/stdlib. In future we may want to add
libraries/gemsto hold third-party libraries. At the moment these live as separate files at thelibrarieslevel.This mirrors the structure followed by the Ruby docs
(https://docs.ruby-lang.org/en/3.1/).
This PR is split up into multiple commits so it's easier to review. As a result, the intermediate commits will not build, but it should make reviewing a lot easier.
The last two commits are optional: they rename the
frameworksdirectory tolibraries, since I think that better describes its contents. This does churn many import paths, so I'm happy to drop this if you think it's too disruptive.