Comment by Matthew Spear (JIRA)

Pretty new to open source contribution, have a forked and built copy of Swift - Would this be an easy bug to jump in and attempt to do, where would you recommend I start?

Comment by Matthew Spear (JIRA)

kkoval (JIRA User) ^^ I'd like to attempt this, just need some help getting started!

matthewspear (JIRA User):

The diagnostic in question is emitted here:

(You can find this by searching for the error message, which leads you to one of the lists of diagnostics:

As for how to fix this, you could probably read the headers for the PatternBindingDecl, which is the particular declaration being examined there (which represents the let x binding, or in this case, just x). It looks like PBD->isImplicit() is already checked, so apparently this one isn't considered "implicit", but I suspect its getLoc() (which is what's being replaced by the fixItReplace) has a misleading value. If the PBD seems to be erroneous, you may need to investigate where it originally came from, which would involve looking through ParseDecl.cpp.

General tips for jumping in / debugging: set lots of breakpoints, read lots of header comments, run lots of whole-project searches, and make test cases as small as possible!

Also, if using Xcode, you'll want to edit your scheme so that ⌘R passes the right command line args to your Swift compiler. Probably -frontend -parse /path/to/your/test-case.swift will be sufficient for working on this.

I've confirmed that this issue still occurs on apple/swift master. Do you mind if I try to fix it, matthewspear (JIRA User)?

Comment by Matthew Spear (JIRA)

Go for it @modocache, I had a look but still getting my head round how Swift works internally with AST - Will be interesting to look at how you fix it. I'm only just getting into open source and sure I can find another starter bug to try fix! Thanks

I enjoy keeping notes as I attempt to solve Swift bugs, so here are some from my work here so far. (I'm confident I'll be able to fix this eventually, so only chime in if you feel like it.)

First of all, this problem can be reduced to:

var x = 1
let y = { [x] in
  x += 1
}

This still produces an incorrect note:

2757.swift:3:5: error: left side of mutating operator isn't mutable: 'x' is a 'let' constant
  x += 1
  ~ ^
2757.swift:2:12: note: change 'let' to 'var' to make it mutable
let y = { [x] in
           ^
           var

Dumping the parse tree for this source file shows that capture list elements are stored as VarDecl:

(source_file
  (top_level_code_decl
    (brace_stmt
      (pattern_binding_decl
        (pattern_named 'x')
        (integer_literal_expr type='<null>' value=1))
))
  (var_decl "x" type='<null type>' storage_kind=stored)
  (top_level_code_decl
    (brace_stmt
      (pattern_binding_decl
        (pattern_named 'y')
        (capture_list type='<null>'
            (var_decl "x" type='<null type>' let storage_kind=stored)
            (pattern_binding_decl
              (pattern_named implicit 'x')
              (unresolved_decl_ref_expr type='<null>' name=x specialized=no) function_ref=unapplied)
          (closure_expr type='<null>' discriminator=0 single-expression
            (parameter_list)
            (sequence_expr type='<null>'
              (declref_expr type='<null>' decl=main.(file).top-level code.x@2757.swift:2:12 function_ref=unapplied specialized=yes)
              (unresolved_decl_ref_expr type='<null>' name=+= specialized=no) function_ref=unapplied
              (integer_literal_expr type='<null>' value=1)))))
))
  (var_decl "y" type='<null type>' let storage_kind=stored))

Capture list elements that are not marked as weak appear to be parsed as let constants. For example, x above is parsed as (var_decl "x" type='<null type>' let storage_kind=stored).

The fact that they're stored as let constants causes the incorrect diagnostic. I figured this out by using lldb:

1. git grep -3 "change 'let' to 'var' to make it mutable" shows that it is a diagnostic named convert_let_to_var.

2. Grepping for convert_let_to_var shows the diagnostic is emitted in VarDecl::emitLetToVarNoteIfSimple.

3. The Swift driver splits up any compilation command into sub-commands. To figure out the sub-command to attach lldb to, I ran swiftc -driver-print-jobs 2757.swift. This outputs swift -frontend -c -primary-file 2757.swift -module-name main -o 2757.o, as well as a second linker sub-command.

4. I ran lldb – swift -frontend -c -primary-file 2757.swift -module-name main -o 2757.o to attach to the Swift compiler, then b VarDecl::emitLetToVarNoteIfSimple to place a breakpoint, then r to run the process, and then bt to spit out a backtrace. This shows the exact methods that are invoked before reaching VarDecl::emitLetToVarNoteIfSimple.

Reading the source code in the functions included in the backtrace, I found that once the TypeChecker has determined that the code is syntactically invalid (DeclChecker::visitPatternBindingDecl → validatePatternBindingDecl → TypeChecker::typeCheckPatternBinding → TypeChecker::typeCheckBinding → TypeChecker::typeCheckExpression → TypeChecker::solveForExpression → ConstraintSystem::salvage), it begins to traverse the broken expression tree in order to emit the most accurate error statement.

diagnoseSubElementFailure actually emits the error statement. It checks various properties on VarDecl: "is the variable immutable? Is it a let constant?" In this case, since the capture list is stored as a let constant VarDecl, the error "'x' is a 'let' constant" is emitted. In turn, when VarDecl::emitLetToVarNoteIfSimple is called, it is happy to add the problematic diagnostic – for all intents and purposes, the TypeChecker thinks it is dealing with a let constant.

So, how to fix this? Well, it seems unlikely that we can change how capture list variables are stored... they've been built to be stored as let constant VarDecls, and I assume the person who wrote it that way had a good reason to do so. I wonder instead if the solution is to differentiate capture list constants from let constants in some way. Maybe an additional method on VarDecl: it already has VarDecl::isImplicit and VarDecl::isLet, so maybe we can add a VarDecl::isCaptureListElement to it...?

I'll probably have time to work on this over the holidays, so expect updates soon.

Alternatively, if there were a way to see the "parent" of the var_decl, I could see that it was a capture_list, and if so I could prevent the fix-it from being printed. I think I've seen a few calls to getParent or getParentMap in the Swift C++ codebase but I don't know how to use these yet.

A couple things off the top of my head:

• Is isImplicit true for bindings like [x]? If not, should it be? If it should be, then is that enough to skip producing the fixit? (Upon further thought, I imagine your best option would be using the "parent", like you said, so the message can be improved too: something like variable bindings in capture lists cannot be mutable)

• It looks like getDeclContext() is usually used for the "parent". Does that work for VarDecls in capture list entries?

Also,

>> I ran lldb – swift -frontend -c -primary-file 2757.swift -module-name main -o 2757.o

I don't know if you use a Mac for development, but personally I find it much easier to use Xcode and its GUI debugging tools than lldb from the command line. You can set up your scheme so that it passes -frontend -c ... as arguments when running the compiler.

Here's my first attempt: #6490 – In the pull request I mention why I couldn't get implicit variables, or walking up the AST, to work.

The pull request was merged, so I think this is done!