[AST][Sema] Typed throws implementation #33653

minuscorp · 2020-08-26T20:33:26Z

This PR adds a basic implementation for adding a type after the throws keyword as discussed in the following thread: https://forums.swift.org/t/typed-throws/39660

This is a draft

Todo:

Add tests
Update mangling (if needed)
Parsing and parse tests.
Type-checking throw type.

minuscorp · 2020-08-26T20:55:50Z

Developers are welcome in order to get this implementation going forward (as it is needed for a review process).

dfsweeney · 2020-08-27T03:47:14Z

I think some of the problems building the stdlib are from a lot of diagnostic noise when trying to parse a type and failing. That's straightforward to fix--you can do something like

    if (!peekToken().isKeyword()) {
      BacktrackingScope backtrackingScope(*this);
      DiagnosticSuppression diagnosticSuppression(Diags);

       ParserResult<TypeRepr> result = parseType();
       if (result.isNonNull()) {
         throwsType = result.get();
       }

when the DiagnosticSuppression goes out of scope the diagnostics go back on. I added that locally and got some improvement.

The one that's generating a lot of noise compiling the stdlib is

   if (Tok.is(tok::arrow)) {
-    ParserResult<TypeRepr> ty =
-      parseTypeSimpleOrComposition(MessageID, HandleCodeCompletion);
-    if (ty.isNull())
-      return ty;
-    auto tyR = ty.get();
-    auto status = ParserStatus(ty);
     // Handle type-function if we have an arrow.
     SourceLoc arrowLoc = consumeToken();

If you take that out, then stdlib compiles, but obviously that defeats the purpose of the whole thing.

I want to make sure I understand that one though. If the current token is an arrow, haven't we moved past anything like throws SomeError ? In the old syntax it's like _ body: (UnsafeBufferPointer<C.Element>) throws -> R, in the new syntax it's _ body: (UnsafeBufferPointer<C.Element>) throws SomeError-> R right? If the current token is an arrow then we are past the throw type. Or am I misunderstanding? I might be missing the context.

minuscorp · 2020-08-27T10:06:49Z

The new syntax is as you said body: (UnsafeBufferPointer<C.Element>) throws SomeError-> R that's why I try to get the type using parseTypeOrComposition before the arrow, and after the potential throws being found, and just if that type exists, that can of course not be there because it is optional.

theblixguy · 2020-08-27T11:30:33Z

You can call canParseType() followed by parseType() if the former succeeds.

minuscorp · 2020-08-27T12:30:20Z

As it was an optional token @theblixguy, I chose to get the type if was present or null in other case scenario. With canParseType() I'll always get the TypeRepr but I have to initialize it to nullptr for the non existent case. Is it worth?

theblixguy · 2020-08-27T15:43:43Z

lib/Parse/ParsePattern.cpp

+        ASTContext &Ctx = SF.getASTContext();
+        DiagnosticSuppression SuppressedDiags(Ctx.Diags);
+        backtrackingScope.cancelBacktrack();


Why is this needed?

I use the backtrack for searching of my previous word was a keyword and if it was the throws keyword, but maybe there's some other way to do it.

I think what you should do is something like:

bool hasType = false; { BacktrackingScope backtrack(*this); hasType = canParseType(); } // Parser backtracks after end of scope above if (hasType) { ParserResult<TypeRepr> result = parseType(); throwsType = result.getPtrOrNull(); }

as canParseType() consumes some tokens IIRC.

That is good for consuming the type token! Thank you! But I'm unable to check that the previous token was tok::throws

dfsweeney · 2020-08-27T16:41:04Z

ok, errors.swift fails with:

/Users/dfs/swift-source/swift/test/Parse/errors.swift:47:8: error: unexpected error produced: expected '{' in body of function declaration
  func bar() throws SomeError {}
       ^
/Users/dfs/swift-source/swift/test/Parse/errors.swift:47:20: error: unexpected error produced: consecutive statements on a line must be separated by ';'
  func bar() throws SomeError {}
                   ^
/Users/dfs/swift-source/swift/test/Parse/errors.swift:47:31: error: unexpected error produced: argument passed to call that takes no arguments
  func bar() throws SomeError {}
                              ^

working through that in lldb it looks like

if (!peekToken().isKeyword()) {
   	      BacktrackingScope backtrackingScope(*this);
 	      if (peekToken().is(tok::kw_throws)) {

is skipping the important part--we already consumed the 'throws' and the active token is the throw type. We should try to parse the type immediately, and not look for throws I made a change and am building locally.

minuscorp · 2020-08-27T16:47:32Z

If we don't check for throws we're gonna end up with a messed order of elements, don't we?

dfsweeney · 2020-08-27T16:56:52Z

If we don't check for throws we're gonna end up with a messed order of elements, don't we?

I think the order of elements should be good--we don't need to check for throws because we already found it, is what it looks like. I could still be missing it.

The stdlib won't build after my change so it needs something else. I'm interpreting what's happening after ParsePattern.cpp:840 as:

We found 'throws' (or 'try' or 'throw')
then at 853 we consume the token and set throwsLoc with the token
So we are pointing at whatever is after the throws, either a type, or something else (-> or {, probably, but there are other possibilities)
we call canParseType and if it succeeds we found a type and should then really parse it and put it in throwsType.

I think where the problem is after my change is that canParseType may be interpreting some things that are not actually types but that are legal Swift as if they were types. I have to look in to that more.

minuscorp · 2020-08-27T17:10:16Z

I got it working, I just delay a step the token being consumed in order to check that there's a type or not just after it 😄

minuscorp · 2020-08-27T17:20:14Z

Could we make a quick testrun and a source compatibility on the CI? (tests are passing locally)

Altough I cannot make stdlib to compile, lots of errors (unrelated), to the changes being made (?)

dfsweeney · 2020-08-27T17:32:32Z

Altough I cannot make stdlib to compile, lots of errors (unrelated), to the changes being made (?)

What are the errors like? I am having problems compiling

  mutating func removeAll(
    where shouldBeRemoved: (Element) throws -> Bool) rethrows

  // FIXME: Associated type inference requires these.
  @_borrowed

in RangeReplaceableCollection.swift, I think because the rethrows is using some of the new logic, although I'm not sure.

minuscorp · 2020-08-27T17:41:20Z

Things like:

/Users/minuscorp/Documents/swift-source/swift/stdlib/public/core/UnsafeBufferPointer.swift.gyb:197:14: error: cannot convert value of type '()' to expected condition type 'Bool'
    if n > 0 ? l >= 0 && l < n : l <= 0 && n < l {
       ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

minuscorp · 2020-08-27T20:14:39Z

Now I'm only getting:

error: consecutive declarations on a line must be separated by ';'
  mutating func encode(_ value: Float, forKey key: Key) throws
          ^
          ;

Only throws related errors in the stdlib

dfsweeney · 2020-08-27T20:21:08Z

is that in KeyedEncodingContainerProtocol? The keyword before mutating is throws (the comments don't count) and it's a protocol so throws terminates the previous method signature (or whatever it's called in a protocol.) But it's reading ahead into the mutating and getting confused.

minuscorp · 2020-08-27T21:04:23Z

I'm getting a hard time with Deserialization, I'll push what I have but I does not compile, I cannot find a way to get from the deserialized func to a TypeRepr.

The Protocols are running into issues all as you said, could you take a look at them?

* Added parsing of 'throws (Type)' syntax. New diagnostic if the parsing fails. Logic to parse an optional parenthesized type identifier after the 'throws' keyword. This will not compile the standard library right now because it cannot parse the pattern `func f(_ body: (T) throws -> U) rethrows -> U {return try body(t)}` That pattern is really common in the standard library for things like `withUnsafeBufferPointer`. I'm not sure what the problem is right now there. * Added Parser::parseThrowsType. Edited Parse/errors.swift test to include some throws (SomeError) entries. Parse/errors.swift passes in this form. Stdlib compiles. Parse/async.swift, Syntax/Parser/tree.swift and incrParse/incrementalTransfer.swift fail right now. Also I think I need to set a SourceLoc for the throwsType explicitly. * [Parse] Parse type after throws correctly in function declarations * [Parse] Parse type after throws correctly in function types * Improve documentation of new parsing functions Co-authored-by: Josef Zoller <josef@walterzollerpiano.com>

Update typed throws implementation

Zollerboy1 · 2020-09-04T22:35:22Z

This is all parsing done.

minuscorp · 2020-09-04T22:44:40Z

@swift-ci please smoke test

minuscorp · 2020-09-04T22:44:53Z

@swift-ci please compatibility test

minuscorp · 2020-09-04T22:49:14Z

cc @theblixguy ?

theblixguy · 2020-09-04T22:49:26Z

You need commit access to trigger CI. I’ll do it for you.

theblixguy · 2020-09-04T22:49:35Z

@swift-ci please smoke test

theblixguy · 2020-09-04T22:49:58Z

@swift-ci please test source compatibility

minuscorp · 2020-09-04T22:55:45Z

and how do I get with those permissions? as we're working on a proposal implementation and will need it sometimes, do I ask you?

minuscorp · 2020-09-04T23:02:11Z

Jenkins died before the smoke test even started on OS X

theblixguy · 2020-09-04T23:07:16Z

You can run the tests locally. If you have any problems with that, then I can run CI for you if you ping me.

theblixguy · 2020-09-04T23:07:32Z

@swift-ci please smoke test macOS

theblixguy · 2020-09-04T23:10:34Z

Looks like there’s an infrastructure problem preventing smoke tests on macOS. Let’s wait for it to be resolved.

minuscorp · 2020-09-04T23:45:34Z

Yeah we always test locally before updating the PR but as the CI is the source of truth we want to make sure we didn't screw up anything locally.

minuscorp · 2020-09-05T12:32:23Z

Two questions @theblixguy, why the compat may fail in debug and not in release? which the difference? on the other hand, the smoke tests have failed, it seems, for unrelated things to the changes made, is it possible to repeat them?

theblixguy · 2020-09-05T14:18:13Z

Debug failure is known (been failing for others as well) and is unrelated to your changes, so I wouldn’t worry about it.

@swift-ci please smoke test

minuscorp · 2020-09-05T17:47:00Z

@swift-ci please smoke test cc @theblixguy

theblixguy · 2020-09-05T17:49:07Z

Let’s just run python lint as that failed last time.

@swift-ci please python lint

minuscorp · 2020-09-05T18:10:24Z

first time see that command

theblixguy · 2020-09-05T18:25:09Z

Passed: https://ci.swift.org/view/Pull%20Request/job/swift-PR-python-lint/198/

shahmishal · 2020-10-01T06:17:18Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

Add basic AST representation of the throws type representation

9d8b426

minuscorp marked this pull request as draft August 26, 2020 20:35

theblixguy added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Aug 26, 2020

Green build

ef1c838

minuscorp added 3 commits August 27, 2020 13:45

Backtrack to find the throws keyword

0495fa0

Check with canParseType()

f8717ef

Added syntax tree nodes

d0ffa0d

Syntax update

995f5ac

theblixguy reviewed Aug 27, 2020

View reviewed changes

Parse Type after seeing and consuming throws

e0e3f05

minuscorp and others added 5 commits August 27, 2020 23:45

Progress... I think

f372825

Get type or null

fa5c8b5

Fix errors tests, broke async-syntax ones (compile crash)

a2d2a24

Update lib/Parse/ParsePattern.cpp

d302869

minuscorp and others added 6 commits September 4, 2020 21:45

Fixes for round_trip_tests and syntax tree

1b44273

Fix incremental test

db2c0bd

Fix Syntax/Parser/tree.swift test

62a4272

Merge remote-tracking branch 'origin/master' into typed-throws-dev

2cedd91

Merge branch 'master' into typed-throws-dev

2deb7fe

Merge pull request #11 from minuscorp/typed-throws-dev

d638e48

Update typed throws implementation

Merge branch 'master' into typed-throws-1

798f130

minuscorp changed the title ~~[AST][Sema] Add basic AST representation of the throws type representation~~ [AST][Sema] Typed throws implementation Sep 4, 2020

Fix python_support tests

f0574ca

minuscorp changed the base branch from master to main October 3, 2020 10:01

[AST][Sema] Typed throws implementation #33653

Are you sure you want to change the base?

[AST][Sema] Typed throws implementation #33653

Conversation

minuscorp commented Aug 26, 2020 • edited Loading

minuscorp commented Aug 26, 2020

dfsweeney commented Aug 27, 2020

minuscorp commented Aug 27, 2020

theblixguy commented Aug 27, 2020

minuscorp commented Aug 27, 2020 • edited Loading

theblixguy Aug 27, 2020

Choose a reason for hiding this comment

minuscorp Aug 27, 2020

Choose a reason for hiding this comment

theblixguy Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

minuscorp Aug 27, 2020

Choose a reason for hiding this comment

dfsweeney commented Aug 27, 2020 • edited Loading

minuscorp commented Aug 27, 2020

dfsweeney commented Aug 27, 2020

minuscorp commented Aug 27, 2020 • edited Loading

minuscorp commented Aug 27, 2020 • edited Loading

dfsweeney commented Aug 27, 2020

minuscorp commented Aug 27, 2020

minuscorp commented Aug 27, 2020

dfsweeney commented Aug 27, 2020

minuscorp commented Aug 27, 2020 • edited Loading

Zollerboy1 commented Sep 4, 2020

minuscorp commented Sep 4, 2020

minuscorp commented Sep 4, 2020

minuscorp commented Sep 4, 2020

theblixguy commented Sep 4, 2020

theblixguy commented Sep 4, 2020

theblixguy commented Sep 4, 2020

minuscorp commented Sep 4, 2020

minuscorp commented Sep 4, 2020

theblixguy commented Sep 4, 2020

theblixguy commented Sep 4, 2020

theblixguy commented Sep 4, 2020

minuscorp commented Sep 4, 2020

minuscorp commented Sep 5, 2020

theblixguy commented Sep 5, 2020

minuscorp commented Sep 5, 2020

theblixguy commented Sep 5, 2020

minuscorp commented Sep 5, 2020

theblixguy commented Sep 5, 2020

shahmishal commented Oct 1, 2020

minuscorp commented Aug 26, 2020 •

edited

Loading

minuscorp commented Aug 27, 2020 •

edited

Loading

theblixguy Aug 27, 2020 •

edited

Loading

dfsweeney commented Aug 27, 2020 •

edited

Loading

minuscorp commented Aug 27, 2020 •

edited

Loading

minuscorp commented Aug 27, 2020 •

edited

Loading

minuscorp commented Aug 27, 2020 •

edited

Loading