-
Notifications
You must be signed in to change notification settings - Fork 54
fix(python): Coerce Composite args to Any at call sites #1106
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?
Changes from 8 commits
b58fff3
4d8c6e5
b3ba52e
47fef11
74ed41b
4790c1a
f5f1dea
d8d233d
52f745b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -991,6 +991,22 @@ partial def coerceToAny (ctx : TranslationContext) (expr : Python.expr SourceRan | |
| pure <| mkStmtExprMd (.Hole) | ||
| else pure translated | ||
|
|
||
| /-- Coerce each argument whose corresponding parameter type is Any. | ||
| Arguments aligned with non-Any parameters are kept unchanged. | ||
| When `fd` is `none` or the argument index exceeds the parameter list, | ||
| the argument is left unchanged (we cannot determine the target type). -/ | ||
| partial def coerceArgsToAny (ctx : TranslationContext) | ||
| (args : List (Python.expr SourceRange)) | ||
| (rawTransArgs : List StmtExprMd) | ||
| (fd : Option PythonFunctionDecl) : Except TranslationError (List StmtExprMd) := do | ||
| let paramTypeNames : Array String := match fd with | ||
| | some fd => (fd.args.map fun a => highTypeToPyLauType a.laurelType.val).toArray | ||
| | none => #[] | ||
| (args.zip rawTransArgs).zipIdx.mapM fun ((orig, trans), i) => | ||
| match paramTypeNames[i]? with | ||
| | some ty => if ty == PyLauType.Any then coerceToAny ctx orig trans else pure trans | ||
| | none => pure trans | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Design trade-off introduced in
The Suggestion: add a fixture under Alternative if the trade-off is judged clean: a one-line doc-comment on the |
||
|
|
||
| partial def refineFunctionCallExpr (ctx : TranslationContext) (func: Python.expr SourceRange) : | ||
| Except TranslationError (String × Option (Python.expr SourceRange) × Bool) := do | ||
| match func with | ||
|
|
@@ -1271,7 +1287,8 @@ partial def translateCall (ctx : TranslationContext) | |
| if args.length > funcDecl.args.length then | ||
| throwUserError callRange | ||
| s!"'{name}' called with too many positional arguments: expected at most {funcDecl.args.length}, got {args.length}" | ||
| let trans_posArgs ← args.mapM (translateExpr ctx) | ||
| let rawPosArgs ← args.mapM (translateExpr ctx) | ||
| let trans_posArgs ← coerceArgsToAny ctx args rawPosArgs (some funcDecl) | ||
| let trans_dict ← translateVarKwargs ctx kwords | ||
| let remainingParams := funcDecl.args.drop args.length | ||
| let trans_dictArgs := remainingParams.map fun arg => | ||
|
|
@@ -1302,7 +1319,8 @@ partial def translateCall (ctx : TranslationContext) | |
| else | ||
| let (args, kwords, funcdecl_hasKwargs) ← | ||
| combinePositionalAndKeywordArgs args kwords funcDecl methodName callRange | ||
| let trans_args ← args.mapM (translateExpr ctx) | ||
| let rawTransArgs ← args.mapM (translateExpr ctx) | ||
| let trans_args ← coerceArgsToAny ctx args rawTransArgs funcDecl | ||
| let trans_kwords ← translateKwargs ctx kwords | ||
| let trans_kwords_exprs := | ||
| if kwords.length == 0 then | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # Test: passing a dispatch-created Composite value to a function with untyped parameter. | ||
| # Before the fix, this caused "Impossible to unify Any with Composite" because | ||
| # the factory dispatch produces a Composite-typed value but the function parameter | ||
| # defaults to Any. | ||
| import servicelib | ||
|
|
||
|
|
||
| def use_storage(client): | ||
| client.put_item(Bucket="test", Key="k", Data="v") | ||
|
|
||
|
|
||
| use_storage(servicelib.connect("storage")) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # Test: passing a dispatch-created Composite value as a positional argument | ||
| # alongside **kwargs expansion. This exercises the first coerceArgsToAny call | ||
| # site (the isVarKwargs branch) where positional args precede the dict expansion. | ||
| import servicelib | ||
|
|
||
|
|
||
| def use_client(client, Bucket, Key, Data): | ||
| client.put_item(Bucket=Bucket, Key=Key, Data=Data) | ||
|
|
||
|
|
||
| def call_with_kwargs(): | ||
| extra = {"Bucket": "b", "Key": "k", "Data": "v"} | ||
| use_client(servicelib.connect("storage"), **extra) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # Test: a Composite-typed field (self.client: Storage) is passed to a function | ||
| # with an untyped parameter. The Composite is coerced to Any at the call site, | ||
| # but inside the class method where the field is used directly, dispatch still | ||
| # works because the field retains its Composite type. | ||
| import servicelib | ||
|
|
||
|
|
||
| class StorageUser: | ||
| def __init__(self): | ||
| self.client: Storage = servicelib.connect("storage") | ||
|
|
||
| def do_put(self): | ||
| self.client.put_item(Bucket="b", Key="k", Data="d") |
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.
I will reiterate what I said in the other PRs. In the current modeling, we assume that values of type Any are never composite since toString is a function that does not depend on the heap. Whereas by construction it was not possible to hit this soundness issue before, this PR would concretize the soundness issue by making it possible to cast a composite into an Any.
The solution is obviously to make toString a bodiless procedure and not a bodiless function and declare that this procedure modifies the heap.