Recent update#10
Hidden character warning
Conversation
ChingLongTin
left a comment
ChingLongTin
left a comment
There was a problem hiding this comment.
There is a regression that should be addressed: https://github.com/ChingLongTin/mlscript/pull/10/changes#r3350061642
The remaining are some minor suggestions.
| import "./Block.mls" | ||
| import "./Option.mls" | ||
| import "./CachedHash.mls" | ||
| import "./ShapeSet.mjs" |
There was a problem hiding this comment.
We might want to put Shape and ShapeSet in a single module in a later commit instead of having both files importing each other.
There was a problem hiding this comment.
Yes. We should do it.
| let fullBlk = foldl((acc, e) => concat(acc, e.0))(End(), ...evaledArgs) | ||
| let newArgs = evaledArgs.map(e => Arg(if e.1 is Path then e.1 else throw Error("expected path"))) | ||
| [fullBlk, Call(f, newArgs), mkDyn()] // non staged function and some params unknown | ||
| argShapes.every(staticSet) then // non staged function and params known |
There was a problem hiding this comment.
This seems to make the sorUnknownCall fallback inside sorStaticModuleCall unreachable.
There was a problem hiding this comment.
I think that branch should be redundant. But there is a case that failed. It is probably due to missing metadata. I added a fixme in the latest commit, and maybe you can help figure it out.
There was a problem hiding this comment.
Actually, we seem to need it. Having all static parameters doesn't mean the result is also static, as we may not always be able to reconstruct the shape of the value. This is the reason this test failed:
class C
module Opaque with
fun f() = new C
staged module M with
fun f() = Opaque.f()
M."f_gen"()
because we can't currently reconstruct the class symbol for C.
2 participants
dynmerging