-
-
Notifications
You must be signed in to change notification settings - Fork 658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Abstract abstract #10513
Abstract abstract #10513
Conversation
LGTM. @Simn ? |
src/typing/typer.ml
Outdated
| (MGet, KAbstractImpl ab) | ||
| (MCall _, KAbstractImpl ab) -> | ||
let t = TAbstract (ab,[]) in | ||
let block = mk (TBlock [(get_this ctx p)]) t p in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure TBlock
is required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mk (TCast ((get_this ctx p), None)) t p
also works, but ide hint for abstract
will be this:String
instead of MyAbstract
. Is there a better way to type (get_this ctx p)
with TAbstract
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try this:
Lines 340 to 341 in 3fac24b
let e = get_this ctx p in | |
let e = {e with etype = TAbstract(a,tl)} in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, but still looks like this:String
or this:T
instead of MyAbstract
on hover. Maybe this can be improved in display stuff someway, but maybe TBlock
is fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this can be improved in display stuff someway
Hmm... I don't know. Maybe @Simn knows. But TBlock
is out of place here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes let's not add random TBlock
nodes like that. I can check the display situation, maybe you could add a failing test for it.
Add a display test please. So we can be sure completion works as expected. |
Closes #10482