Skip to content

sema: reject enum element call with payloads as compile-time constant #41796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 12, 2022
Merged

sema: reject enum element call with payloads as compile-time constant #41796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/Sema/CSFix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,9 @@ bool NotCompileTimeConst::diagnose(const Solution &solution, bool asNote) const
// Referencing an enum element directly is considered a compile-time literal.
if (auto *d = solution.resolveLocatorToDecl(locator).getDecl()) {
if (isa<EnumElementDecl>(d)) {
return true;
if (!d->hasParameterList()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please factor the checks out of the fix, as I mentioned on the precious PR - a fix shouldn’t be created if it’s not going to be diagnosed…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do in a follow-up PR! I'm still figuring out how to refactor this code...

Copy link
Contributor

@xedin xedin Mar 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time thinking about this and it's going to be hard to properly detect that argument is non-const because sometimes e.g. leading-dot syntax the overload is resolved long after argument conversion constraint has been simplified (result gets bound, then base gets bound and only after that the member is resolved). I think one way to archive it might to be add a new flag to a type variable e.g. const required which is going to get associated with "argument" type variable and get propagated to the base of the chain and from it to the member.

Also note that the check you added here is incorrect because NotCompileTimeConst would get a base expression as a locator but the leading-dot syntax chain this base belongs to might have more members than just one.

Copy link
Contributor Author

@nkcsgexi nkcsgexi Mar 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not that the check you added here is incorrect because NotCompileTimeConst would get a base expression as a locator but the leading-dot syntax chain this base belongs to might have more members than just one.

Thanks! Not sure I understand the situation you are describing here. Could you give a test case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, something like:

enum E {
  case a
  case b(Int)
}

using .a.b(42) is going to get accepted because the the base is just .a and .b(42) is not checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this following case:

$cat /tmp/t.swift
enum E {
  case a
  case b(Int)
}
func foo(_ c: _const E) {

}
foo(.a.b(3))
foo(E.a.b(3))

I got:

$../build/Ninja-RelWithDebInfoAssert/swift-macosx-x86_64/bin/swift-frontend -typecheck /tmp/t.swift
/tmp/t.swift:8:6: error: enum case 'b' cannot be used as an instance member
foo(.a.b(3))
    ~^
    E
/tmp/t.swift:9:7: error: enum case 'b' cannot be used as an instance member
foo(E.a.b(3))
    ~~^
    E
/tmp/t.swift:9:9: error: expect a compile-time constant literal
foo(E.a.b(3))
        ^

They seem to be rejected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this one:

enum E {
  case a
  case b(Int)

  var c: E { .b(42) }
}

func test(_: _const E) {
}

test(.a.c)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this one seems to be wrongly accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed rdar://90210674

return true;
}
}
}
NotCompileTimeConstFailure failure(solution, locator);
Expand Down
4 changes: 3 additions & 1 deletion test/Sema/const_enum_elements.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// RUN: %target-typecheck-verify-swift

enum CarKind {
static let wagon = "Wagon"
case coupe
case sedan
case other(String)
Expand All @@ -21,7 +22,8 @@ func main() {
drive(.sedan, k2: .coupe)
drive(CarKind.coupe, k2: CarKind.sedan)
drive(CarKind.sedan, k2: CarKind.coupe)
drive(.other(""), k2: .sedan)
drive(.other(""), k2: .sedan) // expected-error {{expect a compile-time constant literal}}
drive(.other(CarKind.wagon), k2: .sedan) // expected-error {{expect a compile-time constant literal}}

drive(.myCoupe, k2: .sedan) // expected-error {{expect a compile-time constant literal}}
drive(.coupe, k2: .myCoupe) // expected-error {{expect a compile-time constant literal}}
Expand Down