Initial language support for lifetime dependence #71069

meg-gupta · 2024-01-22T20:30:39Z

No description provided.

meg-gupta · 2024-01-22T20:31:02Z

Swift Parser changes and support for lifetime dependence in initializers and function types are pending.

meg-gupta · 2024-01-22T23:21:43Z

@swift-ci test

meg-gupta · 2024-01-23T06:24:37Z

@swift-ci test windows platform

xedin

LGTM! I left a few comments inline. @ahoppen and/or @rintaro could you please take a look at the parser changes?

include/swift/AST/DiagnosticsSema.def

include/swift/AST/ExtInfo.h

include/swift/AST/LifetimeDependence.h

xedin · 2024-01-23T22:42:27Z

include/swift/AST/LifetimeDependence.h

+    Value(Identifier name) : Named({name}) {}
+    Value(unsigned index) : Ordered({index}) {}
+    Value() {}
+  } value;


Instead of doing this could we take advantage of inline bit fields here? We'll get a NamedSpecifier, OrderedSpecifier or SelfSpecifier sublcasses and share trailing storage to store name/id similar to #70998?

Can I do this as a follow on commit ?

xedin · 2024-01-23T22:46:07Z

include/swift/AST/Ownership.h

+static inline llvm::StringRef getOwnershipSpelling(ValueOwnership ownership) {
+  switch (ownership) {
+  case ValueOwnership::Default:
+    return "";


Should this be something more concrete i.e. "default"?

Leaving the spelling as is, because it matched both swift and sil.

lib/Sema/TypeCheckDecl.cpp

atrick · 2024-01-24T00:28:19Z

The diagnostics on valid dependence vs. ownership specifiers looks great to me.

Please also disallow a mutate dependence when there is no ownership specifier!

As discussed, we should

implement inference in Sema based on explicit parameter ownership.
mangle function types based on lifetime dependence regardless of whether explicit or inferred.

But those can be follow-up PRs.

meg-gupta · 2024-01-24T03:32:25Z

@swift-ci smoke test

include/swift/AST/Attr.def

lib/Parse/ParseDecl.cpp

slavapestov · 2024-01-24T14:09:08Z

lib/Sema/TypeCheckDecl.cpp

+    if (returnTypeRepr) {
+      if (auto *lifetimeDependentRepr =
+              dyn_cast<LifetimeDependentReturnTypeRepr>(returnTypeRepr)) {
+        lifetimeDependenceInfo = validateLifetimeDependenceInfo(


This should probably be in a separate request evaluator request

can I do that in a follow up PR ?

slavapestov · 2024-01-24T14:09:49Z

You might want to consider landing some of these changes right away, to avoid merge conflicts.

slavapestov · 2024-01-24T14:09:56Z

(Assuming the existing tests still pass)

meg-gupta · 2024-01-24T20:23:45Z

@swift-ci smoke test

rintaro

Parser changes look good. Nothing serious.

lib/Parse/ParseDecl.cpp

include/swift/AST/Attr.def

rintaro · 2024-01-24T21:14:01Z

lib/Parse/ParseDecl.cpp

+      break;
+    }
+    // consume the lifetime dependence kind
+    consumeToken();


Not remembering the location of kind token makes getStartLoc() incorrect, but I don't think it's serious for now. applyAttributesToType() changes the order of specifiers anyway. We should fix them, but not necessary in this PR.

Also, representing single _copy(x, y) as separate LifetimeDependenceSpecifier make it impossible to restore the textual representation of the specifier (when printing), but I think that's also not serious for now.

rintaro · 2024-01-24T21:14:39Z

lib/AST/TypeRepr.cpp

+
+void LifetimeDependentReturnTypeRepr::printImpl(
+    ASTPrinter &Printer, const PrintOptions &Opts) const {
+  printTypeRepr(getBase(), Printer, Opts);


I assume you're going to implement printing specifiers in followups.

include/swift/AST/DiagnosticsParse.def

ahoppen · 2024-01-24T21:39:20Z

include/swift/AST/DiagnosticsParse.def

+      ())
+
+ERROR(expected_rparen_after_lifetime_dependence, PointsToFirstBadToken,
+      "expected ')' after param list in lifetime dependence specifier", ())


Suggested change

"expected ')' after param list in lifetime dependence specifier", ())

"expected ')' after parameter list in lifetime dependence specifier", ())

Also: I don’ t think we have a test case for this diagnostic.

include/swift/AST/DiagnosticsParse.def

include/swift/Parse/Token.h

lib/Parse/ParseDecl.cpp

ahoppen · 2024-01-24T21:51:32Z

test/Parse/explicit_lifetime_dependence_specifiers.swift

+func use(_ x: borrowing BufferView) {}
+
+func deriveMultiView1(_ x: borrowing BufferView, _ y: borrowing BufferView) -> _borrow(x, y) BufferView {
+  if (Int.random(in: 1..<100) % 2 == 0) {


Nitpick: The parentheses aren’t needed in Swift.

ahoppen · 2024-01-24T21:52:15Z

test/Parse/explicit_lifetime_dependence_specifiers.swift

+  return BufferView(y.ptr)
+}
+
+func deriveMultiView2(_ x: borrowing BufferView, _ y: borrowing BufferView) -> _borrow(x) _borrow(y) BufferView {


Is it intentional that it’s allowed to specify _borrow twice? I think we should error here and only allow _borrow(x, y).

I had the same opinion. @atrick suggested not to diagnose this previously.

If it's going to be a problematic grammar, we can ban it.

cc @atrick

test/Parse/explicit_lifetime_dependence_specifiers.swift

…sult position

meg-gupta · 2024-01-24T23:02:07Z

@swift-ci smoke test

meg-gupta · 2024-01-25T03:39:18Z

@swift-ci smoke test Linux

meg-gupta · 2024-01-25T04:23:03Z

@swift-ci test Linux platform

meg-gupta · 2024-01-25T08:44:15Z

@rintaro @ahoppen Follow up to comments on parser changes - #71147. I have left unaddressed ones open to handle later.

…llowup Follow up for C++ parser changes in #71069

meg-gupta requested review from atrick and xedin January 22, 2024 20:33

atrick requested a review from gottesmm January 23, 2024 22:34

xedin approved these changes Jan 23, 2024

View reviewed changes

meg-gupta marked this pull request as ready for review January 23, 2024 23:30

meg-gupta requested review from hborla, slavapestov, ahoppen, bnbarham, CodaFi, DougGregor, hamishknight, rintaro and AnthonyLatsis as code owners January 23, 2024 23:30

meg-gupta force-pushed the lifetimedependencelangattr branch 2 times, most recently from c9220e6 to d7b974d Compare January 24, 2024 03:26

rintaro reviewed Jan 24, 2024

View reviewed changes

include/swift/AST/Attr.def Outdated Show resolved Hide resolved

lib/Parse/ParseDecl.cpp Outdated Show resolved Hide resolved

lib/Parse/ParseDecl.cpp Outdated Show resolved Hide resolved

slavapestov reviewed Jan 24, 2024

View reviewed changes

meg-gupta force-pushed the lifetimedependencelangattr branch 2 times, most recently from 7f2ec44 to abdade5 Compare January 24, 2024 20:21

rintaro approved these changes Jan 24, 2024

View reviewed changes

ahoppen reviewed Jan 24, 2024

View reviewed changes

meg-gupta added 5 commits January 24, 2024 15:01

Add support for LifetimeDependence in C++ parser

d37b0cf

Add Sema support for LifetimeDependence

0cb805b

Add LifetimeDependenceInfo into function type

52def21

Diagnose if lifetime dependence specifiers are not in the function re…

79248a5

…sult position

Add additional lifetime dependence diagnostics

1894b11

meg-gupta force-pushed the lifetimedependencelangattr branch from abdade5 to 1894b11 Compare January 24, 2024 23:01

meg-gupta added a commit to meg-gupta/swift that referenced this pull request Jan 25, 2024

Follow up for C++ parser changes in swiftlang#71069

08146bc

meg-gupta merged commit 28f27c3 into swiftlang:main Jan 25, 2024

meg-gupta added a commit to meg-gupta/swift that referenced this pull request Jan 25, 2024

Follow up for C++ parser changes in swiftlang#71069

36f43fe

meg-gupta mentioned this pull request Jan 25, 2024

Follow up for C++ parser changes in #71069 #71147

Merged

meg-gupta added a commit that referenced this pull request Jan 25, 2024

Merge pull request #71147 from meg-gupta/lifetimedependencelangattrfo…

38dea46

…llowup Follow up for C++ parser changes in #71069

carlos4242 pushed a commit to carlos4242/swift that referenced this pull request May 31, 2024

Follow up for C++ parser changes in swiftlang#71069

db2a0b3

	"expected ')' after param list in lifetime dependence specifier", ())
	"expected ')' after parameter list in lifetime dependence specifier", ())

Initial language support for lifetime dependence #71069

Initial language support for lifetime dependence #71069

Uh oh!

Conversation

meg-gupta commented Jan 22, 2024

Uh oh!

meg-gupta commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meg-gupta commented Jan 22, 2024

Uh oh!

meg-gupta commented Jan 23, 2024

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atrick commented Jan 24, 2024

Uh oh!

meg-gupta commented Jan 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Jan 24, 2024

Uh oh!

slavapestov commented Jan 24, 2024

Uh oh!

meg-gupta commented Jan 24, 2024

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rintaro Jan 24, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

meg-gupta commented Jan 24, 2024

Uh oh!

meg-gupta commented Jan 25, 2024

Uh oh!

meg-gupta commented Jan 25, 2024

Uh oh!

meg-gupta commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

meg-gupta commented Jan 22, 2024 •

edited

Loading

rintaro Jan 24, 2024 •

edited

Loading

meg-gupta commented Jan 25, 2024 •

edited

Loading