Ruby: Desugar for loops as calls to each
#7098
Conversation
|
I will rebase and resolve merge conflicts once #7141 is merged. |
hvitved
force-pushed
the
ruby/desugar-for-1
branch
from
b764900 to
08c7782
Compare
Commit 028ef6f had the unintended side-effect that the AST and CFG stages got merged, because the AST stage's `isCapturedAccess` now depends on `getCfgScopeImpl`, which belongs to the CFG stage. The fix is to remove `getCfgScopeImpl` from the CFG stage, and instead let it be part of the AST stage.
There was a problem hiding this comment.
Nice work! That's been on the TODO list for a long time.
I can see why – there are a lot of changes, and it would be difficult for me to understand them all and really review them in detail. But overall it makes sense, and all the test changes that I did look into were sensible.
| result = s | ||
| or | ||
| not s instanceof MethodBase and | ||
| not s instanceof ModuleBase and |
There was a problem hiding this comment.
Why is this line needed? (Maybe add a comment?)
There was a problem hiding this comment.
I guess it is not needed, since a module cannot be inside a method (right?)
There was a problem hiding this comment.
It looks like singleton classes can be inside methods. Here's one example I found: https://github.com/gitlabhq/gitlabhq/blob/94ecc00f47df7051eea905a5899053bf476e0589/app/services/users/signup_service.rb#L27-L32
There was a problem hiding this comment.
A module can be created in the body of a method, however, from a scope point of view the method and the module are unrelated.
This is in order to avoid name clash with the often so-named IPA type for data- flow nodes. The name clash is not problematic because they are both in scope, but because (cached) IPA types with overlapping names are known to sometimes result in re-evaluation of cached stages, when one of the IPA types gets an internal `#2` suffix in one query run, and the other IPA type gets the suffix in another run.
Desugars:
into
This, roughly, is what MRI does. Desugaring these lets us drop the custom control flow graph handling for
forloops and instead rely on "standard" modelling of calls toeach.I'm mostly satisfied with the shape of the AST, couple of minor notes:
Blockarguments toMethodCallSynthare given index-2, as-1denotes a desugared version of the call,0the receiver, and1,2,3,...any arguments.0..n-1to represent its parameters, and childrenn..mto represent the statements in its body, wherenis the number of parameters to the block, andm - n + 1is number of statements in the block body - i.e.Parameters, thenStmts