> On Dec 31, 2016, at 2:47 PM, Jacob Bandes-Storch <jtban...@gmail.com> wrote: > > 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 > > Jacob > > On Sat, Dec 31, 2016 at 1:31 PM, Robert Widmann <devteam.cod...@gmail.com > <mailto:devteam.cod...@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.cod...@gmail.com >> <mailto:devteam.cod...@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 >>>> <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 >>>> >>>> <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 >>>> <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 >>> <https://lists.swift.org/mailman/listinfo/swift-dev> > >
_______________________________________________ swift-dev mailing list swift-dev@swift.org https://lists.swift.org/mailman/listinfo/swift-dev
