Leading dot selection (without chained selections yet) #354
Conversation
LPTK
left a comment
LPTK
left a comment
There was a problem hiding this comment.
Nice! But I have a bunch of comments. Also, it seems the CI is failing.
| if usesResTmp then k(Value.Ref(l)) | ||
| else k(unit) // * it seems this currently never happens | ||
| ) | ||
| val sym = resolveField(nme, preTrm.symbol, nme) | ||
| Term.SynthSel(preTrm, nme)(sym, N) | ||
| case Sel(Empty(), nme) => | ||
| Term.Sel(Term.LeadingDotTarget, nme)(N, N, S(ctx)) |
There was a problem hiding this comment.
If we're going to add a new term, it should be Term.LeadingDotSel(nme), tro reduce the amount of illegal states that are representable.
There was a problem hiding this comment.
That makes sense, and I've implemented it, but there arises a problem with Lowering again. If I understand correctly, leading dot selections should not be lowered at all, but the term function in Lowering.scala still gets called, and when the term was Sel(LeadingDotTarget, nme), this was fine since the term function handles the Sel case. However, on LeadingDotSel... what should it do..?
There was a problem hiding this comment.
You just elaborate (expand) it so to make sure no LeadingDotSel appears after the flow analysis stage!
There was a problem hiding this comment.
and probably you would want to add a case in the term function, so that if a LeadingDotSel attempts to get itself lowered, the compiler crashes by calling lastWords (internal error)
| ), | ||
| if (nullary) then nullaryType else Bot, | ||
| ) | ||
| // (binary, unary, nullary) match |
There was a problem hiding this comment.
I actually prefer this version.
There was a problem hiding this comment.
what are you referring to? The pattern match or the if block? if it's the latter, do you mean that we should assign the signature only based on the nullary flag?
| case Flow(sym: FlowSymbol) | ||
| case Fun(lhs: Consumer, rhs: Producer, captures: Ls[(Producer, Consumer)]) | ||
| case Tup(elems: Ls[Opt[SpreadKind] -> Producer]) | ||
| case Tup(elems: Ls[(Opt[SpreadKind], Producer)]) |
There was a problem hiding this comment.
Isn't the -> version easier to read?
There was a problem hiding this comment.
Personally, -> looks to me like a function type, so (,) is much more readable. But the call is entirely yours of course, I will follow the project style
hkmc2/shared/src/main/scala/hkmc2/semantics/flow/FlowAnalysis.scala
Outdated
Show resolved
Hide resolved
| case CompanionMember(_, sym) => msg"companion member ${sym.nme}" -> sym.toLoc | ||
|
|
||
| def solveConstraints() = | ||
| def findConsumerSymbols(cons: Consumer): Ls[Symbol] = |
There was a problem hiding this comment.
Why are we trying to extract consumer symbols from things like tuples and function left/right-hand sides? 🤔
There was a problem hiding this comment.
This is the sort of generalization I was talking about, and how resolutions like .b(new B) work, i.e. by solving the constraint _?_.b <: ((B) => <something>) and traversing the consumer of this constraint to find the type B and its module, where b can be found. The findConsumerSymbols function grabs at any symbols found in the consumer and returns them to be inspected for companion modules. Of course, this raises the problem of conflicting methods, which is why you can toggle this off, and we may remove this feature entirely.
| case ObjectMember(sym) => msg"object member ${sym.nme}" -> sym.toLoc | ||
| case CompanionMember(_, sym) => msg"companion member ${sym.nme}" -> sym.toLoc | ||
| msg"Cannot resolve selection" -> sel.toLoc :: Nil | ||
| // * An error should alsoready be reported in this case |
There was a problem hiding this comment.
"alsoready" kind of makes sense, but is unfortunately not a word.
There was a problem hiding this comment.
that's your comment, not mine :)
| Type.Error | ||
|
|
||
| def typeParam(p: Param): C = | ||
| val fs = FlowSymbol(p.sym.name) |
There was a problem hiding this comment.
Not a fan of this. We should access flow symbols that are reachable from the parameter, so that these can be retrieved later.
There was a problem hiding this comment.
Makes sense. What led me to this was the inability to define a flow symbol directly in the Param class, due to a lack of contextual State in that scope. Would it be better to add an Option[FlowSymbol] field in Param, and set this field to S(FlowSymbol(p.sym.name)) in the typeParam function?
3 participants
We use flow analysis to resolve selections of the form
.method, where the target is unknown, based on the type into which this selection flows. Examples includeSee the
flows/LeadingDotAccesses.mlsfile for the examples.This "generalization" seen in the example with
class Bcan be toggled on and off, as it has both the advantage of flexibility and the disadvantage of potentially conflicting selection resolution candidates. This toggle is currently just a flag in theFlowAnalysis.scalafile, and it might be a good idea to have it as part of the user interface, similarly to a language pragma in Haskell.This is a draft PR, I have the following remaining tasks in mind:
.a_1.a_2.<...>.a_n, which doesn't work out of the box unfortunately :(TODOs and???s that should be properly implemented