CSGen/LinkedExprAnalyzer questions


(Jacob Bandes-Storch) #1

This is a pretty great bug: https://bugs.swift.org/browse/SR-3483

    let x: Double? = 1

    // error: ambiguous reference to member '+'
    let sum = x! + x! + x!

    // error: value of optional type 'Double?' not unwrapped; did you mean
to use '!' or '?'?
    let sum: Double = x! + x! + x!

I've been poking around and I think the problem might be in
LinkedExprAnalyzer. This class collects type info about elements in a chain
of binary operators to speed up constraint solving. It does so in this case
by grabbing the DeclRefExpr's type:
https://github.com/apple/swift/tree/474096b9cbd6ff7ac998d7cea41d629512e25570#L230-L239

However, since this is an ASTWalker, (I think) it automatically traverses
into the ForceValueExpr without remembering that the type it finds inside
(from the DeclRefExpr) should have one level of optionality removed when
added to the constraint system.

This theory sort of makes sense to me, but it doesn't explain why the
simpler "let sum = x! + x!" typechecks correctly, because that goes through
the same code path.

Am I correct that the LinkedExprAnalyzer probably needs to make sure it
doesn't keep the Optional when adding the type of a ForceValueExpr? Why
wouldn't this also cause problems for a single binop application?

Would it be more appropriate for LinkedExprAnalyzer to be an ASTVisitor
(ExprVisitor) so that instead of just saying "yes, continue traversing
downwards" (by returning {true, expr}), its ForceValueExpr case could
recursively call visit() and then getAnyOptionalObjectType on the result?

Semi-eptly,
Jacob


(Mark Lacey) #2

This is a pretty great bug: https://bugs.swift.org/browse/SR-3483

    let x: Double? = 1

    // error: ambiguous reference to member '+'
    let sum = x! + x! + x!

    // error: value of optional type 'Double?' not unwrapped; did you mean to use '!' or '?'?
    let sum: Double = x! + x! + x!

I've been poking around and I think the problem might be in LinkedExprAnalyzer.

Yeah I think you’re onto something there. Just looking at the output of the constraint solver and where bindings are happening in matchTypes(), it looks like the constraint optimizer is trying to force the result type of both adds to ‘Double?’ rather than ‘Double’, so we only ever try to solve for that type.

I haven’t looked at the constraint optimizer code in a while, so I don’t have a lot of insight into the best fix, but it’s clearly the problematic piece here.

Mark

···

On Dec 30, 2016, at 2:07 PM, Jacob Bandes-Storch via swift-dev <swift-dev@swift.org> wrote:

This class collects type info about elements in a chain of binary operators to speed up constraint solving. It does so in this case by grabbing the DeclRefExpr's type: https://github.com/apple/swift/tree/474096b9cbd6ff7ac998d7cea41d629512e25570#L230-L239

However, since this is an ASTWalker, (I think) it automatically traverses into the ForceValueExpr without remembering that the type it finds inside (from the DeclRefExpr) should have one level of optionality removed when added to the constraint system.

This theory sort of makes sense to me, but it doesn't explain why the simpler "let sum = x! + x!" typechecks correctly, because that goes through the same code path.

Am I correct that the LinkedExprAnalyzer probably needs to make sure it doesn't keep the Optional when adding the type of a ForceValueExpr? Why wouldn't this also cause problems for a single binop application?

Would it be more appropriate for LinkedExprAnalyzer to be an ASTVisitor (ExprVisitor) so that instead of just saying "yes, continue traversing downwards" (by returning {true, expr}), its ForceValueExpr case could recursively call visit() and then getAnyOptionalObjectType on the result?

Semi-eptly,
Jacob
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev


(Robert Widmann) #3

I taught LinkedExprAnalyzer about ForceValueExpr

      if (auto FVE = dyn_cast<ForceValueExpr>(expr)) {
        LTI.collectedTypes.insert(CS.getType(FVE)->lookThroughAllAnyOptionalTypes().getPointer());
        return { false, expr };
      }

This seems to get it - didn’t check whether this regresses things, but this seems to be the right fix at a high level.

~Robert Widmann

···

On Dec 31, 2016, at 11:22 AM, Mark Lacey via swift-dev <swift-dev@swift.org> wrote:

On Dec 30, 2016, at 2:07 PM, Jacob Bandes-Storch via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

This is a pretty great bug: https://bugs.swift.org/browse/SR-3483

    let x: Double? = 1

    // error: ambiguous reference to member '+'
    let sum = x! + x! + x!

    // error: value of optional type 'Double?' not unwrapped; did you mean to use '!' or '?'?
    let sum: Double = x! + x! + x!

I've been poking around and I think the problem might be in LinkedExprAnalyzer.

Yeah I think you’re onto something there. Just looking at the output of the constraint solver and where bindings are happening in matchTypes(), it looks like the constraint optimizer is trying to force the result type of both adds to ‘Double?’ rather than ‘Double’, so we only ever try to solve for that type.

I haven’t looked at the constraint optimizer code in a while, so I don’t have a lot of insight into the best fix, but it’s clearly the problematic piece here.

Mark

This class collects type info about elements in a chain of binary operators to speed up constraint solving. It does so in this case by grabbing the DeclRefExpr's type: https://github.com/apple/swift/tree/474096b9cbd6ff7ac998d7cea41d629512e25570#L230-L239

However, since this is an ASTWalker, (I think) it automatically traverses into the ForceValueExpr without remembering that the type it finds inside (from the DeclRefExpr) should have one level of optionality removed when added to the constraint system.

This theory sort of makes sense to me, but it doesn't explain why the simpler "let sum = x! + x!" typechecks correctly, because that goes through the same code path.

Am I correct that the LinkedExprAnalyzer probably needs to make sure it doesn't keep the Optional when adding the type of a ForceValueExpr? Why wouldn't this also cause problems for a single binop application?

Would it be more appropriate for LinkedExprAnalyzer to be an ASTVisitor (ExprVisitor) so that instead of just saying "yes, continue traversing downwards" (by returning {true, expr}), its ForceValueExpr case could recursively call visit() and then getAnyOptionalObjectType on the result?

Semi-eptly,
Jacob
_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev


(Robert Widmann) #4

Whoops, should only look through one level of optionality. You get the gist.

~Robert Widmann

···

On Dec 31, 2016, at 2:31 PM, Robert Widmann <devteam.codafi@gmail.com> wrote:

I taught LinkedExprAnalyzer about ForceValueExpr

      if (auto FVE = dyn_cast<ForceValueExpr>(expr)) {
        LTI.collectedTypes.insert(CS.getType(FVE)->lookThroughAllAnyOptionalTypes().getPointer());
        return { false, expr };
      }

This seems to get it - didn’t check whether this regresses things, but this seems to be the right fix at a high level.

~Robert Widmann

On Dec 31, 2016, at 11:22 AM, Mark Lacey via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Dec 30, 2016, at 2:07 PM, Jacob Bandes-Storch via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

This is a pretty great bug: https://bugs.swift.org/browse/SR-3483

    let x: Double? = 1

    // error: ambiguous reference to member '+'
    let sum = x! + x! + x!

    // error: value of optional type 'Double?' not unwrapped; did you mean to use '!' or '?'?
    let sum: Double = x! + x! + x!

I've been poking around and I think the problem might be in LinkedExprAnalyzer.

Yeah I think you’re onto something there. Just looking at the output of the constraint solver and where bindings are happening in matchTypes(), it looks like the constraint optimizer is trying to force the result type of both adds to ‘Double?’ rather than ‘Double’, so we only ever try to solve for that type.

I haven’t looked at the constraint optimizer code in a while, so I don’t have a lot of insight into the best fix, but it’s clearly the problematic piece here.

Mark

This class collects type info about elements in a chain of binary operators to speed up constraint solving. It does so in this case by grabbing the DeclRefExpr's type: https://github.com/apple/swift/tree/474096b9cbd6ff7ac998d7cea41d629512e25570#L230-L239

However, since this is an ASTWalker, (I think) it automatically traverses into the ForceValueExpr without remembering that the type it finds inside (from the DeclRefExpr) should have one level of optionality removed when added to the constraint system.

This theory sort of makes sense to me, but it doesn't explain why the simpler "let sum = x! + x!" typechecks correctly, because that goes through the same code path.

Am I correct that the LinkedExprAnalyzer probably needs to make sure it doesn't keep the Optional when adding the type of a ForceValueExpr? Why wouldn't this also cause problems for a single binop application?

Would it be more appropriate for LinkedExprAnalyzer to be an ASTVisitor (ExprVisitor) so that instead of just saying "yes, continue traversing downwards" (by returning {true, expr}), its ForceValueExpr case could recursively call visit() and then getAnyOptionalObjectType on the result?

Semi-eptly,
Jacob
_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev


(Jacob Bandes-Storch) #5

CS.getType(FVE) already gets rid of the optionality, so the
lookThrough/getAnyOptionalObjectType isn't necessary at all.

I'm still not sure whether this is the right solution. Are there no other
expression types that need similar treatment? Is ASTWalker really the right
tool for this job? These are questions for someone who knows CSGen better
than I...

Jacob

···

On Sat, Dec 31, 2016 at 1:31 PM, Robert Widmann <devteam.codafi@gmail.com> wrote:

Whoops, should only look through one level of optionality. You get the
gist.

~Robert Widmann

On Dec 31, 2016, at 2:31 PM, Robert Widmann <devteam.codafi@gmail.com> > wrote:

I taught LinkedExprAnalyzer about ForceValueExpr

      if (auto FVE = dyn_cast<ForceValueExpr>(expr)) {
        LTI.collectedTypes.insert(CS.getType(FVE)->lookThroughAllAny
OptionalTypes().getPointer());
        return { false, expr };
      }

This seems to get it - didn’t check whether this regresses things, but
this seems to be the right fix at a high level.

~Robert Widmann

On Dec 31, 2016, at 11:22 AM, Mark Lacey via swift-dev < > swift-dev@swift.org> wrote:

On Dec 30, 2016, at 2:07 PM, Jacob Bandes-Storch via swift-dev < > swift-dev@swift.org> wrote:

This is a pretty great bug: https://bugs.swift.org/browse/SR-3483

    let x: Double? = 1

    // error: ambiguous reference to member '+'
    let sum = x! + x! + x!

    // error: value of optional type 'Double?' not unwrapped; did you
mean to use '!' or '?'?
    let sum: Double = x! + x! + x!

I've been poking around and I think the problem might be in
LinkedExprAnalyzer.

Yeah I think you’re onto something there. Just looking at the output of
the constraint solver and where bindings are happening in matchTypes(), it
looks like the constraint optimizer is trying to force the result type of
both adds to ‘Double?’ rather than ‘Double’, so we only ever try to solve
for that type.

I haven’t looked at the constraint optimizer code in a while, so I don’t
have a lot of insight into the best fix, but it’s clearly the problematic
piece here.

Mark

This class collects type info about elements in a chain of binary
operators to speed up constraint solving. It does so in this case by
grabbing the DeclRefExpr's type: https://github.com/apple/swift/tree/
474096b9cbd6ff7ac998d7cea41d629512e25570#L230-L239

However, since this is an ASTWalker, (I think) it automatically traverses
into the ForceValueExpr without remembering that the type it finds inside
(from the DeclRefExpr) should have one level of optionality removed when
added to the constraint system.

This theory sort of makes sense to me, but it doesn't explain why the
simpler "let sum = x! + x!" typechecks correctly, because that goes
through the same code path.

Am I correct that the LinkedExprAnalyzer probably needs to make sure it
doesn't keep the Optional when adding the type of a ForceValueExpr? Why
wouldn't this also cause problems for a single binop application?

Would it be more appropriate for LinkedExprAnalyzer to be an ASTVisitor
(ExprVisitor) so that instead of just saying "yes, continue traversing
downwards" (by returning {true, expr}), its ForceValueExpr case could
recursively call visit() and then getAnyOptionalObjectType on the result?

Semi-eptly,
Jacob
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev


(Robert Widmann) #6

CS.getType(FVE) already gets rid of the optionality, so the lookThrough/getAnyOptionalObjectType isn't necessary at all.

Ah, sorry, quite right.

I'm still not sure whether this is the right solution. Are there no other expression types that need similar treatment? Is ASTWalker really the right tool for this job? These are questions for someone who knows CSGen better than I…

At the moment that would be Joe Pamer, but there’s some... conflicts that occur going down that path. As for why this works with non-chained binops no simplification occurs in that case because, well, there’s no chain here. The solver walks each part in turn instead.

The walker is, I think, appropriate. It is a very domain-specific optimization that doesn’t quite know about its entire domain yet. However, I don’t think anyone would be opposed to a little meta-programming with an ASTVisitor to refactor it into a more manageable form.

~Robert Widmann

···

On Dec 31, 2016, at 2:47 PM, Jacob Bandes-Storch <jtbandes@gmail.com> wrote:

Jacob

On Sat, Dec 31, 2016 at 1:31 PM, Robert Widmann <devteam.codafi@gmail.com <mailto:devteam.codafi@gmail.com>> wrote:
Whoops, should only look through one level of optionality. You get the gist.

~Robert Widmann

On Dec 31, 2016, at 2:31 PM, Robert Widmann <devteam.codafi@gmail.com <mailto:devteam.codafi@gmail.com>> wrote:

I taught LinkedExprAnalyzer about ForceValueExpr

      if (auto FVE = dyn_cast<ForceValueExpr>(expr)) {
        LTI.collectedTypes.insert(CS.getType(FVE)->lookThroughAllAnyOptionalTypes().getPointer());
        return { false, expr };
      }

This seems to get it - didn’t check whether this regresses things, but this seems to be the right fix at a high level.

~Robert Widmann

On Dec 31, 2016, at 11:22 AM, Mark Lacey via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

On Dec 30, 2016, at 2:07 PM, Jacob Bandes-Storch via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

This is a pretty great bug: https://bugs.swift.org/browse/SR-3483

    let x: Double? = 1

    // error: ambiguous reference to member '+'
    let sum = x! + x! + x!

    // error: value of optional type 'Double?' not unwrapped; did you mean to use '!' or '?'?
    let sum: Double = x! + x! + x!

I've been poking around and I think the problem might be in LinkedExprAnalyzer.

Yeah I think you’re onto something there. Just looking at the output of the constraint solver and where bindings are happening in matchTypes(), it looks like the constraint optimizer is trying to force the result type of both adds to ‘Double?’ rather than ‘Double’, so we only ever try to solve for that type.

I haven’t looked at the constraint optimizer code in a while, so I don’t have a lot of insight into the best fix, but it’s clearly the problematic piece here.

Mark

This class collects type info about elements in a chain of binary operators to speed up constraint solving. It does so in this case by grabbing the DeclRefExpr's type: https://github.com/apple/swift/tree/474096b9cbd6ff7ac998d7cea41d629512e25570#L230-L239

However, since this is an ASTWalker, (I think) it automatically traverses into the ForceValueExpr without remembering that the type it finds inside (from the DeclRefExpr) should have one level of optionality removed when added to the constraint system.

This theory sort of makes sense to me, but it doesn't explain why the simpler "let sum = x! + x!" typechecks correctly, because that goes through the same code path.

Am I correct that the LinkedExprAnalyzer probably needs to make sure it doesn't keep the Optional when adding the type of a ForceValueExpr? Why wouldn't this also cause problems for a single binop application?

Would it be more appropriate for LinkedExprAnalyzer to be an ASTVisitor (ExprVisitor) so that instead of just saying "yes, continue traversing downwards" (by returning {true, expr}), its ForceValueExpr case could recursively call visit() and then getAnyOptionalObjectType on the result?

Semi-eptly,
Jacob
_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev

_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev


(Jacob Bandes-Storch) #7

As for why this works with non-chained binops no simplification occurs in
that case because, well, there’s no chain here. The solver walks each part
in turn instead.

What do you mean by this? The LinkedExprAnalyzer is still used in the "x! +
x!" case. Maybe I just haven't followed the execution quite far enough to
see why the current behavior doesn't cause problems.

···

On Sat, Dec 31, 2016 at 1:55 PM, Robert Widmann <devteam.codafi@gmail.com> wrote:

The walker is, I think, appropriate. It is a very domain-specific
optimization that doesn’t quite know about its entire domain yet. However,
I don’t think anyone would be opposed to a little meta-programming with an
ASTVisitor to refactor it into a more manageable form.

~Robert Widmann