[AST] Introduce ArgumentList #38836

hamishknight · 2021-08-11T14:40:22Z

Introduce the ArgumentList type, which represents a set of call arguments for a function or subscript. This supersedes the use of tuple and paren exprs as argument lists, a relic from when tuple splatting was a language feature.

For now, paren and tuple types retain their ParameterTypeFlags, as both the constraint system and a small amount of diagnostic logic is still relying on converting argument lists into tuple or paren types, this will be removed in a follow-up. This then sets the stage for the removal of InOutExpr and InOutType.

I've split the commits up based on the part of the compiler they touch, please feel free to review the bits of the compiler you're most familiar with.

hamishknight · 2021-08-11T14:43:26Z

@nathawes Mind taking a look at the changes specifically in Formatting.cpp?

slavapestov · 2021-08-11T16:34:45Z

Nice work!

CodaFi

Taken a few passes over this and I haven't seen anything so far. Sema, AST, Parse, and SIL changes all LGTM. CS LGTM but would prefer @hborla and @xedin give their rubber stamp there.

nathawes

The Formatting.cpp changes LGTM. It's so great this is finally happening – thanks a lot!

xedin

Looks great! I have left a small comment about ArgumentLists that could be addressed separately.

xedin · 2021-08-12T18:49:53Z

include/swift/Sema/ConstraintSystem.h

-  /// that locator.
-  llvm::DenseMap<ConstraintLocator *, ArgumentInfo> ArgumentInfos;
+  /// constructions) to the argument lists for the call to that locator.
+  llvm::DenseMap<ConstraintLocator *, ArgumentList *> ArgumentLists;


While we are at it I think we need to do change this to behave like addNodeTypes and record ArgumentLists in the solution as well. Since constraint generation for result builders happens during solving this list would effectively get rewritten multiple times with the out-of-scope ArgumentList objects just to keep them around until constraint system lifetime ends.

xedin · 2021-08-12T18:51:38Z

lib/Sema/CSDiagnostics.cpp

-  return getAsExpr(simplifyLocatorToAnchor(argListLoc));
+ArgumentList *
+FailureDiagnostic::getArgumentListFor(ConstraintLocator *locator) const {
+  return getConstraintSystem().getArgumentList(locator);


See my comment about ArgumentLists - It'd be great if we could extract this information for Solution instead because we can't reply on constraint system having solution applied always e.g. ambiguity cases.

bnbarham

Awesome 😍! The refactoring changes look good to me (two comments but it's fine as is).

lib/IDE/Refactoring.cpp

hamishknight · 2021-08-13T16:30:04Z

Compiler performance numbers are just noise, Jenkins is sporadically crashing during some of the source compat suite runs. Previous performance runs didn't show any meaningful differences though, only a slim decrease in AST memory allocation.

benlangmuir

🎉 lib/Index, lib/IDE, ASTWalker, and SourceKit all LGTM. I made one suggestion in SyntaxModel, but it doesn't need to block the PR, I think. I love how much horrible code this removed.

lib/IDE/SourceEntityWalker.cpp

lib/IDE/SyntaxModel.cpp

rintaro

lib/Parse, lib/IDE, SourceKit changes LGTM!

lib/IDE/SourceEntityWalker.cpp

rintaro · 2021-08-13T23:51:42Z

lib/Parse/ParseStmt.cpp

+                           lpLoc, yieldElts, rpLoc, SyntaxKind::ExprList);
+    for (auto &elt : yieldElts) {
+      assert(elt.Label.empty());
+      assert(elt.LabelLoc.isInvalid());


(Unrelated to this PR)
Hmm, I just noticed the following code hits these assertion even with current main.

struct S { var property: String { _read { yield(x: "test") } } }

Instead of skipping parsing labels in parseExprList(), we probably should diagnose parsed labels here. Or, we should not use parseExprList. (Filed https://bugs.swift.org/browse/SR-15066)

Good catch! Looks like the logic in parseExprList is trying to handle this case, but isn't working as SyntaxKind::ExprList is being passed (though looking at the git history, this seems to have been changed to fix the syntax tree). I can take a look at this in a follow-up.

hamishknight · 2021-08-17T20:24:09Z

Stress tester run uncovered a nasty bug in the ASTWalker logic, now fixed

beccadax

This is going to be great! I've only looked at ArgumentList.h so far, but here are some initial thoughts. Sorry if you've already addressed some of this previously.

include/swift/AST/ArgumentList.h

beccadax · 2021-08-19T07:14:35Z

include/swift/AST/ArgumentList.h

+  /// Note that in type-checked AST, this may return \c true for an index of a
+  /// variadic expansion which contains a trailing closure (and it may not
+  /// necessarily be the first argument of the expansion).
+  bool isRawTrailingClosureIndex(unsigned i) const {


isTrailingClosureArgument()?

Hmm, I think calling out the fact that it's a "raw" index is important, as it's not what you may expect in type-checked AST, in particular it might return true for default arguments or arguments that have been re-ordered after the trailing closure.

Alternatively we may just want to completely ban calling this on a type-checked argument list, what do you think?

With the original arguments update this becomes isTrailingClosureIndex. Personally I prefer that over isTrailingClosureArgument, as that sounds like it would expect an Argument parameter, but I'm happy to change it over

include/swift/AST/ArgumentList.h

Introduce the ArgumentList type, which represents a set of call arguments for a function or subscript. This will supersede the use of tuple and paren exprs as argument lists.

Switch out the representation of argument lists in various AST nodes with ArgumentList.

Split up the expr list parsing members such that there are separate entry points for tuple and argument list parsing, and start using the argument list parsing member for call and subscripts.

With the removal of TupleExpr and ParenExpr argument lists, it's no longer sufficient to check whether a given parent expr has a possible chain sub-expr, as it might be a child that's unrelated to the chain under consideration. For example, for a chain that's an argument to a function call, we don't want to walk up and consider the call expr to be an extension of the chain just because it has a function expr. Tighten up the check by comparing whether the given sub-expr of a possible chain parent matches the current expr in the chain.

- Explicitly limit favoring logic to only handle unary args, this seems to have always been the case, but needs to be handled explicitly now that argument lists aren't exprs - Update the ConstraintLocator simplification to handle argument lists - Store a mapping of locators to argument lists in the constraint system - Abstract more logic into a getArgumentLocator method which retrieves an argument-to-param locator from an argument anchor expr

Most of this should be fairly mechanical, the changes in PlaygroundTransform are a little more involved though.

Track the depth and clear out the map when it reaches 0 to save memory.

We were accidentally continuing to walk the old argument list even if the caller had returned a new one.

- Remove OriginalArguments in favor of storing the pre-rewritten argument list, which simplifies things nicely - Adopt llvm::indexed_accessor_iterator

hamishknight · 2021-09-01T17:52:11Z

@ahoppen Mind taking a look at a1f127f?

hamishknight · 2021-09-01T17:52:20Z

@swift-ci please test

hamishknight · 2021-09-01T17:52:32Z

@swift-ci please test source compatibility

hamishknight · 2021-09-01T17:52:44Z

@swift-ci please test compiler performance

hamishknight · 2021-09-01T17:52:54Z

@swift-ci please SourceKit stress test

hamishknight · 2021-09-03T08:20:45Z

Stress tester result is a UPASS:

05:31:20 SourceKit Stress Tester summary:
05:31:20   failed underlying source compatibility build
05:31:20       > treat this as a source compatibility failure
05:31:20   4 XFails not processed
05:31:20   0 unexpected stress tester failures
05:31:20   135 expected stress tester failures tracked by 17 issues

The source compat failures are already known

ahoppen

a1f127f LGTM.

hamishknight · 2021-09-04T18:29:59Z

🚢

hamishknight requested review from CodaFi, beccadax, xedin, hborla, benlangmuir, nathawes, rintaro and gottesmm August 11, 2021 14:41

CodaFi approved these changes Aug 11, 2021

View reviewed changes

nathawes approved these changes Aug 11, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

xedin approved these changes Aug 12, 2021

View reviewed changes

bnbarham approved these changes Aug 13, 2021

View reviewed changes

lib/IDE/Refactoring.cpp Show resolved Hide resolved

lib/IDE/Refactoring.cpp Show resolved Hide resolved

This comment has been minimized.

Sign in to view

benlangmuir approved these changes Aug 13, 2021

View reviewed changes

lib/IDE/SourceEntityWalker.cpp Outdated Show resolved Hide resolved

lib/IDE/SyntaxModel.cpp Show resolved Hide resolved

hamishknight force-pushed the toil-and-tuple branch from 4174484 to 406d321 Compare August 13, 2021 20:36

rintaro mentioned this pull request Aug 12, 2021

[CodeCompletion] Explicitly support enum pattern matching #38627

Merged

rintaro approved these changes Aug 13, 2021

View reviewed changes

hamishknight force-pushed the toil-and-tuple branch from 406d321 to 9ecbcc2 Compare August 17, 2021 20:22

beccadax reviewed Aug 19, 2021

View reviewed changes

hamishknight mentioned this pull request Aug 27, 2021

[Sema] Have isValidTypeExprParent consider its child #39073

Merged

hamishknight added 5 commits September 1, 2021 18:40

[AST] Introduce ArgumentList

7c7c531

Introduce the ArgumentList type, which represents a set of call arguments for a function or subscript. This will supersede the use of tuple and paren exprs as argument lists.

[AST] Adopt ArgumentList

01a082a

Switch out the representation of argument lists in various AST nodes with ArgumentList.

[Parser] Adopt ArgumentList

a577548

Split up the expr list parsing members such that there are separate entry points for tuple and argument list parsing, and start using the argument list parsing member for call and subscripts.

hamishknight added 9 commits September 1, 2021 18:40

[MiscDiagnostics] Adopt ArgumentList

ad55c09

[IDE] Adopt ArgumentList

78d8d09

[CodeSynthesis] Adopt ArgumentList

4775482

Most of this should be fairly mechanical, the changes in PlaygroundTransform are a little more involved though.

[Sema] Adopt ArgumentList

0574da3

[SIL] Adopt ArgumentList

cead30d

[SyntaxModel] Clear ArgumentInfo when outside of an argument list

488e91a

Track the depth and clear out the map when it reaches 0 to save memory.

[ASTWalker] Fix ArgumentList walking bug

74ecacc

We were accidentally continuing to walk the old argument list even if the caller had returned a new one.

[Sema] Update getMatchVarIfInPatternMatch

a1f127f

Update for review feedback

3e28bbb

- Remove OriginalArguments in favor of storing the pre-rewritten argument list, which simplifies things nicely - Adopt llvm::indexed_accessor_iterator

hamishknight force-pushed the toil-and-tuple branch from 9ecbcc2 to 3e28bbb Compare September 1, 2021 17:42

hamishknight requested a review from beccadax September 1, 2021 17:51

ahoppen approved these changes Sep 3, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

hamishknight merged commit dc60996 into swiftlang:main Sep 4, 2021

hamishknight deleted the toil-and-tuple branch September 4, 2021 18:30

This was referenced Nov 8, 2021

[CSGen] Don't walk into ?? in LinkedExprAnalyzer. #40078

Merged

[CS] Don't favor based on tuple element types #40083

Merged

ahoppen mentioned this pull request May 3, 2022

[Refactoring] Fix an issue where refactoring misses to refactor references after prefix operators #58613

Merged

[AST] Introduce ArgumentList #38836

[AST] Introduce ArgumentList #38836

Uh oh!

Conversation

hamishknight commented Aug 11, 2021

Uh oh!

hamishknight commented Aug 11, 2021

Uh oh!

slavapestov commented Aug 11, 2021

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

nathawes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

xedin Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

hamishknight commented Aug 13, 2021

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rintaro Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamishknight Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamishknight commented Aug 17, 2021

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beccadax Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

hamishknight Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

hamishknight Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hamishknight commented Sep 1, 2021

Uh oh!

hamishknight commented Sep 1, 2021

Uh oh!

hamishknight commented Sep 1, 2021

Uh oh!

hamishknight commented Sep 1, 2021

rintaro Aug 13, 2021 •

edited

Loading

hamishknight Aug 16, 2021 •

edited

Loading