pointfreeco / swift-case-paths

🧰 Case paths extends the key path hierarchy to enum cases.
https://www.pointfree.co/collections/enums-and-structs/case-paths
MIT License
904 stars 105 forks source link

Compiler Crash with generic called `Element` #182

Closed iwt-dahlborn closed 1 month ago

iwt-dahlborn commented 1 month ago

Describe the bug When you use a generic with the name Element within an enum with the CasePathable macro the compiler throws an error.

To Reproduce

To reproduce this error you can simply use a Generic with the name Element in a CasePathable enum

This causes a compiler error

import CasePaths

@CasePathable
enum Action<Element> {
   case elements(Element)
}

this on the other hand does not cause the compiler error

import CasePaths

@CasePathable
enum Action<ActionElement> {
   case elements(ActionElement)
}

It also happens if for example the parent type defines the generic

// fails
struct Parent<Element> {
    @CasePathable
    enum Action {
        case element(Element)
    }
}
// compiles just fine
struct Parent<ParentElement> {
    @CasePathable
    enum Action {
        case element(ParentElement)
    }
}

Expected behavior Either for the Element variant to compile or for CasePathable to forbid using it with an error message.

Environment

Additional context To test this I simply created a new Xcode project in Xcode 15.4 added only swift-case-paths as a dependency and added the examples at the top.

Compiler Stacktrace

``` 1. Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4) 2. Compiling with the current language version 3. While evaluating request ASTLoweringRequest(Lowering AST to SIL for file "REDACTED") 4. While silgen emitFunction SIL function "@$s19CasePathMinExample_6ActionO03AllA5PathsV8elements0aG003AnyaB0VyACyxGs010PartialKeyB0CyAG0A0VyAJGGGvg". for getter for elements (at @__swiftmacro_19CasePathMinExample_6Action0A8PathablefMm_.swift:8:16) Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it): 0 swift-frontend 0x00000001088a7f3c llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56 1 swift-frontend 0x00000001088a70f8 llvm::sys::RunSignalHandlers() + 112 2 swift-frontend 0x00000001088a8544 SignalHandler(int) + 360 3 libsystem_platform.dylib 0x0000000184d13584 _sigtramp + 56 4 swift-frontend 0x00000001038db0f0 swift::ASTVisitor<(anonymous namespace)::StmtEmitter, void, void, void, void, void, void>::visit(swift::Stmt*) + 3604 5 swift-frontend 0x00000001038db0f0 swift::ASTVisitor<(anonymous namespace)::StmtEmitter, void, void, void, void, void, void>::visit(swift::Stmt*) + 3604 6 swift-frontend 0x00000001038db6f4 swift::ASTVisitor<(anonymous namespace)::StmtEmitter, void, void, void, void, void, void>::visit(swift::Stmt*) + 5144 7 swift-frontend 0x0000000103872538 swift::Lowering::SILGenFunction::emitFunction(swift::FuncDecl*) + 632 8 swift-frontend 0x00000001037c6e48 swift::Lowering::SILGenModule::emitFunctionDefinition(swift::SILDeclRef, swift::SILFunction*) + 8344 9 swift-frontend 0x00000001037c75bc swift::Lowering::SILGenModule::emitOrDelayFunction(swift::SILDeclRef) + 172 10 swift-frontend 0x00000001037c4d9c swift::Lowering::SILGenModule::emitFunction(swift::FuncDecl*) + 292 11 swift-frontend 0x00000001038ef988 void llvm::function_ref::callback_fn<(anonymous namespace)::SILGenType::visitAccessors(swift::AbstractStorageDecl*)::'lambda'(swift::AccessorDecl*)>(long, swift::AccessorDecl*) + 36 12 swift-frontend 0x00000001038ef918 (anonymous namespace)::SILGenType::visitAbstractStorageDecl(swift::AbstractStorageDecl*) + 364 13 swift-frontend 0x00000001038ef794 (anonymous namespace)::SILGenType::visitVarDecl(swift::VarDecl*) + 832 14 swift-frontend 0x00000001038eb6d8 (anonymous namespace)::SILGenType::emitType() + 188 15 swift-frontend 0x00000001038eb6c8 (anonymous namespace)::SILGenType::emitType() + 172 16 swift-frontend 0x00000001037c49b8 swift::ASTVisitor::visit(swift::Decl*) + 104 17 swift-frontend 0x00000001037cab54 swift::ASTLoweringRequest::evaluate(swift::Evaluator&, swift::ASTLoweringDescriptor) const + 1668 18 swift-frontend 0x00000001038d9f1c swift::SimpleRequest> (swift::ASTLoweringDescriptor), (swift::RequestFlags)9>::evaluateRequest(swift::ASTLoweringRequest const&, swift::Evaluator&) + 196 19 swift-frontend 0x00000001037cdf44 llvm::Expected swift::Evaluator::getResultUncached(swift::ASTLoweringRequest const&) + 584 20 swift-frontend 0x00000001031a5408 swift::performCompileStepsPostSema(swift::CompilerInstance&, int&, swift::FrontendObserver*) + 548 21 swift-frontend 0x00000001031a9694 performCompile(swift::CompilerInstance&, int&, swift::FrontendObserver*) + 1448 22 swift-frontend 0x00000001031a76d0 swift::performFrontend(llvm::ArrayRef, char const*, void*, swift::FrontendObserver*) + 4968 23 swift-frontend 0x0000000103136e8c swift::mainEntry(int, char const**) + 2612 24 dyld 0x000000018495a0e0 start + 2360 ```

mbrandonw commented 1 month ago

Hi @iwt-dahlborn, good catch! Thanks for the report. This will be fixed by #183.

iwt-dahlborn commented 1 month ago

Hey @mbrandonw, thank you for the quick fix.

I was testing and the compiler still crashes for the variant were the generic parameter clause is on the Parent.

struct Reducer<Element> {
    @CasePathable
    enum Action {
        case element(Element)
    }
}

As well as if the associated value uses Element as its own generic

@CasePathable
enum Action<Element> {
    case element(Array<Element>)
}

Also if the type of your associated value is just called Element

struct Element {
    let value: String
}

@CasePathable
enum Action {
    case element(Element)
}

if you prefer I can also submit this as a separate issue.

mbrandonw commented 1 month ago

Hi @iwt-dahlborn, bummer. Yeah this is a lot more complicated unfortunately. The reason the compiler is crashing is because we making the AllCasePaths type conform to Sequence so that you can get a sequence of all case paths in a type. The Sequence protocol has an Element associated type, and that is conflicting with your Element generic. And there is no way in Swift to disambiguate these types.

We fixed this by introducing a typealias _$Element = Element to the enum when using an Element generic, but that doesn't help if the Element generic is coming from a parent type. Macros have no way of seeing that, and so in those cases there's not much we can do. And in your Array<Element> situation there is also not much we can do since we can't change that to Array<_$Element>.

So, there really is no complete fix here. Even if we banned Element generics from being used in @CasePathables you would still be able to write problematic code. We'll have to think about this a bit more.

iwt-dahlborn commented 1 month ago

Would it be possible to just blanket generate a typealias for each associated value using the unique name feature in macros? That would avoid any checks that would be required and would also avoid any future collisions, right?

Or do you think that would add too much overhead?

mbrandonw commented 1 month ago

I'm not entirely sure I see how that can work. Can you sketch an example of the macro expanded code? You could start from a sample from the snapshot tests in the library.

Another potential option is to not conform AllCasePaths to Sequence and instead introduce a secondary type. Just a bummer to little people's enums with yet another type.

iwt-dahlborn commented 1 month ago

@mbrandonw I created a rough pull request with the general idea: #185

mbrandonw commented 1 month ago

Ah yes, when you said "associated value" I was incorrectly thinking "associated type" of the protocol rather than the payloads of the enum. That certainly is interesting, and we will discuss it along with other options. Thanks for taking the time to draft that PR!