[AST] Fix for SR-7: Inferring closure param type to inout crashes compiler

[gregomni]

Diagnose a variable inferred to be inout, but without explicit ‘&’. This fixes https://bugs.swift.org/browse/SR-7.

[gregomni]

Added a test, and ran all the tests.

[lattner]

Cool, thanks for tackling this. That said, I don't think that this is the right solution. It looks like the generated AST is:

   (closure_expr type='(inout Int) -> ()' location=t.swift:6:9 range=[t.swift:6:9 - line:6:22] discriminator=0 single-expression
      (call_expr type='()' location=t.swift:6:16 range=[t.swift:6:16 - line:6:20] nothrow
        (declref_expr type='(inout Int) -> ()' location=t.swift:6:16 range=[t.swift:6:16 - line:6:16] decl=t.(file)[email protected]:4:6 specialized=no)
        (paren_expr type='(inout Int)' location=t.swift:6:19 range=[t.swift:6:18 - line:6:20]
          (declref_expr type='inout Int' location=t.swift:6:19 range=[t.swift:6:19 - line:6:19] decl=t.(file).top-level code.explicit closure disarm

In this case, the better solution is to fix it so the DeclRefExpr isn't getting inout type in the first place. Something in the formation of the CaptureExpr isn't properly converting the DRE's type to @lvalue Int.

[lattner]

btw, you can see this AST with: swiftc -parse -dump-ast t.swift

[gregomni]

Yeah, I should've asked explicitly about that. Let me restate what I think you are saying: What is happening is that the constraint solver expects the argument to f() to be inout Int, so it matches the type variable for x to be inout Int. That looked dodgy to me, but I wasn't certain whether it ought to be expected or not, so the fix here looks at it after type solving is complete and then complains.

What ought to happen instead is that inout should never be part of the constraint in the first place, so x should be constrained to Int, and then it'll hit the existing missing ampersand check.

Which makes perfect sense and would be better, and is going to be harder for me to figure out how to do correctly. :-)

[lattner]

yep, exactly! A DeclRefExpr should never have InOutType. This would also be a great thing to add to the ASTVerifier if you're interested.

[gregomni]

Okay, this looks better. All tests pass, with some changed error messages (as you can see in the test diffs). I think all are better except that it would be nice if expressions.swift:748 had the older error instead - now the compiler is thinking the result of the ternary ought to be Int and an ampersand is missing and complaining about that type mismatch instead of the old more direct errors. (Maybe that diagnostic should walk into expressions that are themselves error type?)

You can see via the error messages, or via turning on -debug-constraints that the DeclRefs are getting the correct types now, and I've also added the never InOutType check to the verifier.

[lattner]

This sounds very promising, don't worry too much about diagnostics changes, I can improve them in a follow-up patch. Can you please squash these patches into one diff I can review? Thanks!

[lattner]

Oh never mind, I see how to see them as a unit...

[lattner]

Here are a couple nit-picky comments:

• Please keep the indentation level at 2 spaces, in AST/Verifier.cpp
• In a boolean context, please use "isa" instead of "dyn_cast". If you are interested in these, their behavior is described here: http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates
• Feel free to propose the verifier patch as a standalone patch separate from the other work. Have you run the test suite to make sure it doesn't expose any other breakage?

More significantly, I'm not certain that the change you're proposing to the type checker is the right place to do it. Instead of putting it in constraint inference, it seems more natural to go into the place which is assigning a type to the DeclRefExpr. I will poke at this more to try to understand it better (I am not an expert on this part of the constraint solver) and get back to you.

[lattner]

Ok, after digging in a bit more, this is yet-another really unfortunate issue related to how we're modeling "&" in the AST. IMO it shouldn't be an explicit expr, and we shouldn't have inout type at all, the & should be part of the ApplyExpr representation for the call. However, that is a medium term rework of much of the compiler that is on my todo list, so not really relevant to getting this issue fixed :-)

I think the best and simplest place to enforce this is in CSApply.cpp, in buildDeclRef. I think you should be able to add this check that "!type->is()" between these lines at the bottom:

  auto type = simplifyType(openedType);
  return new (ctx) DeclRefExpr(decl, loc, implicit, semantics, type);

And then have it produce an error message. Getting a useful error message will be tricky, but the first order of business is to fix the crash. Thank you for working on this!

-Chris

[gregomni]

Thanks, I'll go take a look there! I agree, since the "&" explicitly can't be used anywhere besides directly at the call param site it seems like having inout as part of the type (anywhere besides Params) gets in the way a lot more than it helps.

[gregomni]

Fixed and commits squashed and ran tests! This was definitely the right place to do it, thanks for the hint, and sorry for the false starts.

[lattner]

Fantastic, looks great. Minor nit-picky stuff, we usually capitalize local variable contractions like "TC" for type checker. I'll fix that in a followup patch, thank you for doing this!

-Chris

[lattner]

Please also close SR-7 when you have a chance, thanks again!

[lattner]

actually, I retract that, I see that file is using lowercase "tc" consistently. Thank you for following its local policy.

[jrose-apple]

Thanks for the fix! Nitpick: in the future, please make the "title" (first line) of your commit message stand on its own, so that others don't have to read the full message or go to the bug tracker to know what's going on.

[gregomni]

Will do, thanks!