-
Notifications
You must be signed in to change notification settings - Fork 833
Fix: specified type is not checked for use! binding #19009
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
base: main
Are you sure you want to change the base?
Conversation
❗ Release notes requiredCaution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository:
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request. You can open this PR in browser to add release notes: open in github.dev
|
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.
The naive approach of simply copying the type node doesn't look correct to me, because of how types of the bound expressions and the patterns rarely match.
Please let's not merge it until a proper explanation is provided.
match typeAnnOpt with | ||
| Some(ty, mTy) -> | ||
// Apply the type annotation to the identifier expression to surface type errors on the annotation | ||
SynExpr.Typed(SynExpr.Ident ident, ty, unionRanges ident.idRange mTy) | ||
| None -> SynExpr.Ident ident |
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.
@edgarfgp Could you explain this logic in greater detail, please?
There're very tricky details related to the types in computation expression builder methods, so I think it would be nice if everyone could follow your thought process 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.
It also seems to follow that such code will compile, even though it is erroneous.
open System
async {
use! (x: string): IDisposable = async { return null }
return x
}
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.
@auduchinok My current attempt is to move the annotation off the Bind lambda parameter to the value we pass to Using so we don't force Bind type parameter(We let Bind pick the right input type from overloads by leaving the lambda parameter untyped.), but we still type-check the user’s annotation and report errors. But as Alex shared this wont be enough. My goal here is to find the minimal fix for this issue. Not to redesign the ExprAsUseBang
case.
Description
Fixes #19005
Checklist