Fix duplicate label issue, include index for duplicate effect labels #513
Conversation
| if labelName l1 == labelName l2 | ||
| then numbered ((i+1,l2):(i, l1):acc) labs | ||
| else numbered ((0,l2):(i, l1):acc) labs | ||
| numbered acc [] = reverse acc |
There was a problem hiding this comment.
Here is where I number the duplicate labels (they are presorted, so labels with the same name will already be next to each other).
| let coreMask = coreExprFromNameInfo maskQName maskInfo | ||
| coreIndex= Core.App (Core.TypeApp (coreExprFromNameInfo evvIndexQName evvIndexInfo) [effTo]) | ||
| [coreHTag] | ||
| [(makeInt32 0), coreHTag] |
There was a problem hiding this comment.
Not sure about this one, this is in inference of Inject creating the core statement @inject-effect. I assumed the new effect will be the first one of that tag, but I'm not sure if I understand this part of inference that well currently.
| findMatch :: Integer -> Name -> [Type] -> Integer | ||
| findMatch i lname (l:ls) = if (labelName l == lname) then i else findMatch (i+1) lname ls | ||
| findMatch :: Integer -> (Int, Name) -> [(Int, Type)] -> Integer | ||
| findMatch i (off, lname) ((ol,l):ls) = if (labelName l == lname) && off == ol then i else findMatch (i+1) (off, lname) ls |
There was a problem hiding this comment.
This changes to find the label with the correct name at the correct count of the label name.
| bottomUp (App (TypeApp (Var evvIndex _) [effTp,hndTp]) [htag]) | getName evvIndex == nameEvvIndex && isEffectFixed effTp | ||
| = makeEvIndex (effectOffset (effectLabelFromHandler hndTp) effTp) | ||
| bottomUp (App (TypeApp (Var evvIndex _) [effTp,hndTp]) [App _ [Lit (LitInt i)], htag]) | getName evvIndex == nameEvvIndex && isEffectFixed effTp | ||
| = makeEvIndex (effectOffset ((fromIntegral i), (effectLabelFromHandler hndTp)) effTp) |
There was a problem hiding this comment.
Of course simplification has to take into account the new parameter
| = let (htagTp,hndTp) | ||
| = let (name,_,tpArgs) = labelNameEx l | ||
| hndCon = TCon (TypeCon name -- (toHandlerName name) | ||
| (kindFunN (map getKind tpArgs ++ [kindEffect,kindStar]) kindStar)) | ||
| in (makeTypeApp (resolve (toEffectTagName name)) tpArgs, typeApp hndCon tpArgs) | ||
| in App (makeTypeApp (resolve nameEvvIndex) [effTo,hndTp]) [htagTp] | ||
| in App (makeTypeApp (resolve nameEvvIndex) [effTo,hndTp]) [makeInt32 (fromIntegral i), htagTp] |
There was a problem hiding this comment.
This is the only change to open resolve. Adding in the name's count.
| if (kk_string_cmp_borrow(htag.tagname,ev->htag.tagname,ctx) <= 0) return i; // break on insertion point | ||
| if (kk_string_cmp_borrow(htag.tagname,ev->htag.tagname,ctx) <= 0) { | ||
| if (off == 0) return i; // break on insertion point | ||
| off--; // adjust for duplicate label |
There was a problem hiding this comment.
Here is the main change in the runtime. We get the handler with the correct name at the correct offset.
| @@ -540,7 +541,7 @@ inferIsolated contextRange range body inf | |||
| Nothing -> return res | |||
| Just vrng -> do seff <- subst ieff | |||
| let (ls,tl) = extractOrderedEffect seff | |||
| case filter (\l -> labelName l == nameTpLocal) ls of | |||
| case filter (\l -> labelName l == nameTpLocal) (map snd ls) of | |||
There was a problem hiding this comment.
All other changes just look like this essentially (getting the result of extractOrderedEffect as it was previously.
TimWhiting
force-pushed
the
fix-duplicate-labels
branch
from
412f1e5
to
be019fe
Compare
Fixes #511
@daanx, I understand if you would like to redo this yourself to make it cleaner, but this should illustrate the issue / solution.
Specifically
@evv-indexdoes not include an index for duplicate labels. This is fine for most effect expressions, since the effect row is incrementally adjusted. However, for the generalopencase where you use a vector of indices and there are multiple labels of the same kind, we get invalid offsets. This would happen with or without the code inSimplify.hswhich turns them into constant offsets.In general I think the types for effect rows need to represent duplicate label indices more explicitly (when extracting the ordered effect), so I defaulted to including the count of duplicate labels (pseudo index) in
extractOrderedEffect. However, the indices aren't really important till after type checking, so there are a lot of places where I needed to discard the indices. I don't know if you would rather do something to represent this invariant more explicitly in the type, or if you would rather create another function for when you don't care about indices, since there are only a few places that do.In particular the issue is illustrated by the following short code snippet from the core output. (from the example in #511)