[PlaygroundTransform] Add experimental feature PlaygroundExtendedCallbacks to pass more info in callbacks
#67215
Conversation
abertelrud
requested review from
hborla,
slavapestov,
xedin and
tshortli
as code owners
|
@swift-ci please test |
|
@swift-ci please test windows platform |
|
@swift-ci please smoke test |
|
@cwakamo I think you may have looked at this code in the past. Do you have any concern about adding this experimental flag and callback? |
|
@xedin I think you reviewed the previous playground transform PR... do you have any concerns? |
lib/Sema/PlaygroundTransform.cpp
Outdated
|
|
||
| // Use #sourceLocation if it exists. | ||
| SourceLoc sl = SR.Start; | ||
| auto *vf = Context.SourceMgr.getVirtualFile(sl); |
There was a problem hiding this comment.
SourceManager has an operation getDisplayNameForLoc that is used for printing the name in other compiler outputs. Would you prefer to use that?
There was a problem hiding this comment.
Thanks for the suggestion @DougGregor! In this case, what we'd want to pass to the callback is the actual path through which the file can later be opened — the phrase "display name" suggests that this function might not always return a literal path, but from the implementation, it looks as if it does. Ideally we'd want the same path that is emitted in .dia files — from the code, it looks as if that's indeed using getDisplayNameForLoc — is that correct?
There was a problem hiding this comment.
Thanks for the advice. It looks good as far as I can tell. I'll update the PR.
…ore information in `-playground` callbacks Adds the experimental feature `PlaygroundExtendedCallbacks` which (when `-playground` is also passed) causes the playground transform to use alternate forms of the result-value, scope-entry, and scope-exit callbacks that include the module name and file path of the source file. The previous callbacks included integers for the module number and file number, but this was cumbersome to use because it required the caller to create source symbols with magical names formed from the module name and file path that the playground transform knew how to look up. The extended callbacks in the experimental feature instead pass these strings as string literals. This is an experimental feature because of the need to measure the performance impact, and because of the need to provide an option to control which set of callbacks to use so that existing clients of the playground transform can opt into it when ready. It's also likely that we'll want to pass more information in the extended callbacks, such as an indication of the kind of result is being logged (e.g. a loop iteration variable vs a return statement vs a variable assignment). So this should be considered the first of a series of experimental improvements that will then be pitched as an actual, non-experimental v2.0 of the playground transform callback API. Because of the nature of how the playground transform is used, it's much easier to iterate on the functionality in the form of an experimental feature rather than using only desktop debug builds of the Swift compiler. Changes: - define a new experimental feature called `PlaygroundExtendedCallbacks` - modify the playground transform step in sema to pass the module name and file name literals when the experimental feature is set - add a unit test for the extended callbacks There is no change in behaviour when `PlaygroundExtendedCallbacks` is not enabled. rdar://109911742
…ameForLoc()` function instead.
abertelrud
force-pushed
the
eng/anders/playground-transform-extended-callbacks
branch
from
7e4073a to
ac1c1dc
Compare
|
@swift-ci please smoke test |
|
Linux failure looks unrelated. Trying again. |
|
@swift-ci please smoke test linux |
1 similar comment
|
@swift-ci please smoke test linux |
abertelrud
deleted the
eng/anders/playground-transform-extended-callbacks
branch
Summary
Adds the experimental feature
PlaygroundExtendedCallbackswhich (when-playgroundis also passed) causes the playground transform to use alternate forms of the result-value, scope-entry, and scope-exit callbacks that include the module name and file path of the source file.Rationale
The previous callbacks included integers for the module number and file number, but this was cumbersome to use because it required the caller to create source symbols with magical names formed from the module name and file path that the playground transform knew how to look up.
The extended callbacks in the experimental feature instead pass these strings as string literals. This is an experimental feature because of the need to measure the performance impact, and because of the need to provide an option to control which set of callbacks to use so that existing clients of the playground transform can opt into it when ready. It's also likely that we'll want to pass more information in the extended callbacks, such as an indication of the kind of result is being logged (e.g. a loop iteration variable vs a return statement vs a variable assignment). So this should be considered the first of a series of experimental improvements that will then be pitched as an actual, non-experimental v2.0 of the playground transform callback API. Because of the nature of how the playground transform is used, it's much easier to iterate on the functionality in the form of an experimental feature rather than using only desktop debug builds of the Swift compiler.
Changes
PlaygroundExtendedCallbacksThere is no change in behaviour of the playground transform when
PlaygroundExtendedCallbacksis not enabled.rdar://109911742