The crash was caused by attempting to add logging expressions that capture generic variables while using the outer func decl context that was not the generic decl context of the inner (generic) func.
The fix "pushes" the current func decl context while instrumenting the body. Rather than always using the top-level func decl context for all nested func bodies.
When a decl that has properties with accessors (`get`, `set`, `willSet`, `didSet`, `subscript()`, etc) is nested inside another type, those accessor implementations aren't playground-transformed.
The reason is that the playground transform uses an `ASTWalker` to get to the top-level structure, but then once it’s inside a type, it does directly nested `transformDecl()` calls. And that inner check is missing a case for accessors.
This change adds an else-clause that checks if the declaration is a `AbstractVarDecl`, and if so, calls `transformDecl()` on each accessor.
It's unfortunate that the playground transform reaches decls in two different ways (using an `ASTWalker` to get to the top-level decls but then using directly nested calls below). That issue seems to be worth resolving at some point (perhaps by using the `ASTWalker` for the whole traversal?)... but that would be a larger change requiring more thought, and so the missing results being fixed here are worth addressing with a safer short-term fix.
rdar://139656464
The problem here is when wrapping a type that has constructors and destructors inside another type.
The reason is that the playground transform uses an `ASTWalker` to get to the top-level structure, but then once it’s inside a type, it does directly nested transformDecl() calls. And that inner check was for too narrow of a type.
The problem in this case was that the `dyn_cast<FuncDecl>(D)` was too narrow and didn’t include constructors/destructors. We want `dyn_cast<AbstractFunctionDecl>(D)`.
We should probably resolve this duality (using an `ASTWalker` to get to the top-level decls but then using directly nested calls below) at some point, but that’s a larger change, and so the specific problem covered by this commit is worth addressing with this safer short-term fix.
Changes:
- change a `dyn_cast<FuncDecl>` to a `dyn_cast<AbstractFunctionDecl>` in PlaygroundTransform.cpp
- add a unit test nested init and deinit (this test also tests the unnested case)
rdar://137316110
When ImplicitOpenExistentials was enabled (default in Swift language mode 6) the Instrumenter would crash the compiler when building logger calls. This was due to an incorrect assumption that the newly created apply expr wouldn't change type when type-checked. However, the type checker is free to change the kind of expression and did so in circumstances where the call expr was wrapped in an open existential expr.
With Swift 6 the print()/debugPrint() function decl is a sub expression of a function conversion expression.
Added tests for print() capture with -swift-version 6.
rdar://136858280
Playgrounds cannot display any macro expansions anyway and the @Observable macro
can generate instrumentations that don't typecheck and/or reference missing
functions.
rdar://112122752
Although I don't plan to bring over new assertions wholesale
into the current qualification branch, it's entirely possible
that various minor changes in main will use the new assertions;
having this basic support in the release branch will simplify that.
(This is why I'm adding the includes as a separate pass from
rewriting the individual assertions)
Skip playground transform for functions that have skipped bodies. This can happen when passing `-emit-module` and `-experimental-skip-non-inlinable-function-bodies-without-types` and also enabling the playground transform. This causes the type to be `nullptr`. The fix is for the playground transform to check `AbstractFunctionDecl::isBodySkipped()`.
rdar://119258854
The "typechecked function body" request was defined to type-check a
function body that is known to be present, and not skipped, and would
assert these conditions, requiring its users to check whether a body
was expected. Often, this means that callers would use `getBody()`
instead, which retrieves the underlying value in whatever form it
happens to be, and assume it has been mutated appropriately.
Make the "typechecked function body" request, triggered by
`getTypecheckedBody()`, more resilient and central. A `NULL` result is
now acceptable, signifying that there is no body. Clients will need to
tolerate NULL results.
* When there is no body but should be one, produce an appropriate
error.
* When there shouldn't be a body but is, produce an appropriate error
* Handle skipping of function bodies here, rather than elsewhere.
Over time, we should move clients off of `getBody` and `hasBody`
entirely, and toward `getTypecheckedBody` or some yet-to-be-introduced
forms like `getBodyAsWritten` for the pre-typechecked body.
Generalize the existing `-playground-high-performance` flag into a set of options that control various aspects of the "playground transformation" in Sema.
This commit adds the first two of those controllable parts of the transform, matching what the existing flag already controls (scope entry/exit and function arguments), but in an extensible way. The intent is for this to be a scalable way to control a larger set of upcoming options.
So instead of a single flag, we represent the playground transform options as a set of well-defined choices, with a new `-playground-option` flag to individually enable or disable those options (when prefixed with "No", the corresponding option is instead disabled). Enabling an already-enabled option or disabling an already-disabled option is a no-op.
For compatibility, the existing `-playground-high-performance` flag causes "expensive" transforms to be disabled, as before. We can also leave it as a useful shorthand to include or exclude new options even in the future, based on their cost. There is a comment on the old function indicating that new code should use the more general form, but it remains for clients like LLDB until they can switch over.
The machinery for implementing the playground options is similar to how `Features.def` works, with a new `PlaygroundOptions.def` that defines the supported playground transform options. Each playground definition specifies the name and description, as well as whether the option is enabled by default, and whether it's also enabled in the "high performance" case.
Adding a new option in the future only requires adding it to `PlaygroundOptions.def`, deciding whether it should be on or off by default, deciding whether it should also be on or off in `-playground-high-performance` mode, and checking for its presence from the appropriate places in `PlaygroundTransform.cpp`.
Note that this is intended to control the types of user-visible results that the invoker of the compiler wants, from an externally detectable standpoint. Other flags, such as whether or not to use the extended form of the callbacks, remain as experimental features, since those deal with the mechanics and not the desired observed behavior.
rdar://109911673
This is a futile attempt to discourage future use of getType() by
giving it a "scary" name.
We want people to use getInterfaceType() like with the other decl kinds.
* Add experimental feature `PlaygroundExtendedCallbacks` which passes more 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
Co-authored-by: Brent Shank <bshank@apple.com>
When an accessor macro adds a non-observing accessor to a property, it
subsumes the initializer. We had previously modeled this as removing
the initializer, but doing so means that the initializer could not be
used for type inference and was lost in the AST.
Explicitly mark the initializer as "subsumed" here, and be more
careful when querying the initializer to distinguish between "the
initializer that was written" and "the initializer that will execute"
in more places. This distinction already existed at the
pattern-binding level, but not at the variable-declaration level.
This is the proper fix for the circular reference issue described in
rdar://108565923 (test case in the prior commit).
Add logging of function and closure parameter values when the playground transform is enabled and its "high-performance" mode is off.
MOTIVATION
The goal of the optional "playground transform" step in Sema is to instrument the code by inserting calls to `__builtin_log()` and similar log functions, in order to record the execution of the compiled code. Some IDEs (such as Xcode) enable this transform by passing -playground and provide implementations of the logger functions that record information that can then be shown in the IDE.
The playground transform currently logs variable assignments and return statements, but it doesn't log the input parameters received by functions and closures. Knowing these values can be very useful in order to understand the behaviour of functions and closures.
CHANGES
- add a `ParameterList` parameter to `InstrumenterSupport::transformBraceStmt()`
- this is an optional parameter list that, if given, specifies the parameters that should be logged at the start of the brace statement
- this has to be passed into the function because it comes from the owner of the BraceStmt
- adjust `PlaygroundTransform.cpp` to make use of this list
- the transform will insert calls to `__builtin_log()` for each of the parameters, in order
- adjust `PCMacro.cpp` to accept the parameter, though this instrumenter doesn't currently make use of the new information
- add two new unit tests (one for functions and one for closures)
- adjust four existing unit tests to account for the new log calls
REMARKS
- this is currently guarded by the existing "high performance" option (parameter logging is omitted in that case)
rdar://104974072
Provide ASTWalker with a customization point to specify whether to
check macro arguments (which are type checked but never emitted), the
macro expansion (which is the result of applying the macro and is
actually emitted into the source), or both. Provide answers for the
~115 different ASTWalker visitors throughout the code base.
Fixes rdar://104042945, which concerns checking of effects in
macro arguments---which we shouldn't do.
Replace the use of bool and pointer returns for
`walkToXXXPre`/`walkToXXXPost`, and instead use
explicit actions such as `Action::Continue(E)`,
`Action::SkipChildren(E)`, and `Action::Stop()`.
There are also conditional variants, e.g
`Action::SkipChildrenIf`, `Action::VisitChildrenIf`,
and `Action::StopIf`.
There is still more work that can be done here, in
particular:
- SourceEntityWalker still needs to be migrated.
- Some uses of `return false` in pre-visitation
methods can likely now be replaced by
`Action::Stop`.
- We still use bool and pointer returns internally
within the ASTWalker traversal, which could likely
be improved.
But I'm leaving those as future work for now as
this patch is already large enough.
Async-let pattern-binding-decls can't be logged since they haven't been
await'd yet. This patch fixes it so the playground transform doesn't try
to log the async-let.
We'll need this to get the right 'selfDC' when name lookup
finds a 'self' declaration in a capture list, eg
class C {
func bar() {}
func foo() {
_ = { [self] in bar() }
}
}
Like switch cases, a catch clause may now include a comma-
separated list of patterns. The body will be executed if any
one of those patterns is matched.
This patch replaces `CatchStmt` with `CaseStmt` as the children
of `DoCatchStmt` in the AST. This necessitates a number of changes
throughout the compiler, including:
- Parser & libsyntax support for the new syntax and AST structure
- Typechecking of multi-pattern catches, including those which
contain bindings.
- SILGen support
- Code completion updates
- Profiler updates
- Name lookup changes
When a force-value expression (!) is encountered while logging
variables, retain the force-value expression rather than dropping it.
This allows logging involving implicitly unwrapped optionals.
Fixes rdar://problem/56098581.
Change the various AST transforms to use prebuilt DeclName constants more heavily rather than an ad-hoc mix of Identifiers, string literals, std::strings, and StringRefs with questionable memory management.
This is an amalgam of simplifications to the way VarDecls are checked
and assigned interface types.
First, remove TypeCheckPattern's ability to assign the interface and
contextual types for a given var decl. Instead, replace it with the
notion of a "naming pattern". This is the pattern that semantically
binds a given VarDecl into scope, and whose type will be used to compute
the interface type. Note that not all VarDecls have a naming pattern
because they may not be canonical.
Second, remove VarDecl's separate contextual type member, and force the
contextual type to be computed the way it always was: by mapping the
interface type into the parent decl context.
Third, introduce a catch-all diagnostic to properly handle the change in
the way that circularity checking occurs. This is also motivated by
TypeCheckPattern not being principled about which parts of the AST it
chooses to invalidate, especially the parent pattern and naming patterns
for a given VarDecl. Once VarDecls are invalidated along with their
parent patterns, a large amount of this diagnostic churn can disappear.
Unfortunately, if this isn't here, we will fail to catch a number of
obviously circular cases and fail to emit a diagnostic.
Like the last commit, SourceFile is used a lot by Parse and Sema, but
less so by the ClangImporter and (de)Serialization. Split it out to
cut down on recompilation times when something changes.
This commit does /not/ split the implementation of SourceFile out of
Module.cpp, which is where most of it lives. That might also be a
reasonable change, but the reason I was reluctant to is because a
number of SourceFile members correspond to the entry points in
ModuleDecl. Someone else can pick this up later if they decide it's a
good idea.
No functionality change.
Most of AST, Parse, and Sema deal with FileUnits regularly, but SIL
and IRGen certainly don't. Split FileUnit out into its own header to
cut down on recompilation times when something changes.
No functionality change.
Accessors logically belong to their storage and can be synthesized
on the fly, so removing them from the members list eliminates one
source of mutability (but doesn't eliminate it; there are also
witnesses for derived conformances, and implicit constructors).
Since a few ASTWalker implementations break in non-trivial ways when
the traversal is changed to visit accessors as children of the storage
rather than peers, I hacked up the ASTWalker to optionally preserve
the old traversal order for now. This is ugly and needs to be cleaned up,
but I want to avoid breaking _too_ much with this commit.
