TypeVar and virtual support in Symbol Resolver #99

Merged
sb10q merged 12 commits from symbol_resolver_typevar into master 2021-12-01 22:44:53 +08:00
Collaborator

Add TypeVar and virtual support for Symbol Resolver in nac3artiq and nac3standalone

Add `TypeVar` and `virtual` support for Symbol Resolver in nac3artiq and nac3standalone
sb10q requested review from pca006132 2021-11-19 12:59:07 +08:00
Contributor

Regarding _eval_type:

  1. It is a private API which may be changed across Python versions. Are there alternatives to this?
  2. How does this function handle name resolution? For example, if the type annotation is A in module X with a class A which is not imported in our current module.

Otherwise LGTM. (if tested, I did not look into details)

Regarding `_eval_type`: 1. It is a private API which may be changed across Python versions. Are there alternatives to this? 2. How does this function handle name resolution? For example, if the type annotation is `A` in module X with a class `A` which is not imported in our current module. Otherwise LGTM. (if tested, I did not look into details)
ychenfo force-pushed symbol_resolver_typevar from 90b7657a72 to dab06bdb58 2021-11-20 05:08:35 +08:00 Compare
Author
Collaborator

Regarding _eval_type:

  1. It is a private API which may be changed across Python versions. Are there alternatives to this?

Yes I also have some conern about this when writing the code. I tried to find a better python API but could not find one so far... This PEP contains some information about this, currently the public API seems to be only typing.get_type_hints, which can only get type hints for function, method, module or class object, but does not seem to work on contraints in TypeVars.
There is an from __future__ import annotations which could also solve this problem, but this should require users to write it in their code.

  1. How does this function handle name resolution? For example, if the type annotation is A in module X with a class A which is not imported in our current module.

I am not sure if I fully got your question. But in this case, if class A is defined in module Y, and module Y is imported to module X, type annotation "X.A" would work. Otherwise there is no way for python to know that is A?

> Regarding `_eval_type`: > 1. It is a private API which may be changed across Python versions. Are there alternatives to this? Yes I also have some conern about this when writing the code. I tried to find a better python API but could not find one so far... This [PEP](https://www.python.org/dev/peps/pep-0563/#resolving-type-hints-at-runtime) contains some information about this, currently the public API seems to be only [`typing.get_type_hints`](https://docs.python.org/3/library/typing.html#typing.get_type_hints), which can only get type hints for function, method, module or class object, but does not seem to work on contraints in `TypeVar`s. There is an `from __future__ import annotations` which could also solve this problem, but this should require users to write it in their code. > 2. How does this function handle name resolution? For example, if the type annotation is `A` in module X with a class `A` which is not imported in our current module. > I am not sure if I fully got your question. But in this case, if class `A` is defined in module `Y`, and module `Y` is imported to module `X`, type annotation `"X.A"` would work. Otherwise there is no way for python to know that is `A`?
Owner

Can we just remove anything that has to do with ForwardRef for now, and deal with the forward type annotation situation later? #73

Can we just remove anything that has to do with ``ForwardRef`` for now, and deal with the forward type annotation situation later? https://git.m-labs.hk/M-Labs/nac3/issues/73
Author
Collaborator

Can we just remove anything that has to do with ForwardRef for now, and deal with the forward type annotation situation later? #73

Ok, I will do that and update this PR.

Edit: removed in the new commit

> Can we just remove anything that has to do with ``ForwardRef`` for now, and deal with the forward type annotation situation later? https://git.m-labs.hk/M-Labs/nac3/issues/73 Ok, I will do that and update this PR. Edit: removed in the new commit
ychenfo added 1 commit 2021-11-21 06:02:31 +08:00
pca006132 approved these changes 2021-11-21 20:01:09 +08:00
sb10q reviewed 2021-11-22 13:07:07 +08:00
@ -17,2 +18,4 @@
pass
# place the `virtual` class infront of the construct of NAC3 object to ensure the
# virtual class is known during the initializing of NAC3 object
Owner

Unclear - do you have an example?

Unclear - do you have an example?
Author
Collaborator

Sorry for the unclear comment. Here I mean that before this line in min_artiq.py: compiler = nac3artiq.NAC3(core_arguments["target"]), the class virtual should already be known to CPython (because the construction of compiler needs to read get the id of it)

if we move this class definition below:

compiler = nac3artiq.NAC3(core_arguments["target"])
class virtual(Generic[T]):
    pass

and run the compiler, we will have:

PyErr { type: <class 'KeyError'>, value: KeyError('virtual'), traceback: None }
Sorry for the unclear comment. Here I mean that before this line in `min_artiq.py`: `compiler = nac3artiq.NAC3(core_arguments["target"])`, the class `virtual` should already be known to CPython (because the construction of `compiler` needs to read get the id of it) if we move this class definition below: ```python compiler = nac3artiq.NAC3(core_arguments["target"]) class virtual(Generic[T]): pass ``` and run the compiler, we will have: ``` PyErr { type: <class 'KeyError'>, value: KeyError('virtual'), traceback: None } ```
Owner

By the way, why is this different than the KernelInvariant class ?

By the way, why is this different than the KernelInvariant class ?
Author
Collaborator

As I understand it, here during the initialization of the nac3 python object it needs to know the id of class virtual becuase virtual[..] is actually a type that can exist in the range of a typevar, and when resolving typevars, nac3artiq needs the id of it to know that it is handling a virtual.

While KernelInvariant in min_artiq.py is just for cpython to not complain about unknown name and we do not need to know its id because so far it only occurs in class fields definition and nac3 do not need to know any cpython related information about it since we are directly looking into the string in the ast.

As I understand it, here during the initialization of the nac3 python object it needs to know the id of class `virtual` becuase `virtual[..]` is actually a type that can exist in the range of a typevar, and when resolving typevars, nac3artiq needs the id of it to know that it is handling a `virtual`. While KernelInvariant in `min_artiq.py` is just for cpython to not complain about unknown name and we do not need to know its id because so far it only occurs in class fields definition and nac3 do not need to know any cpython related information about it since we are directly looking into the string in the ast.
sb10q reviewed 2021-11-22 13:07:26 +08:00
@ -114,0 +111,4 @@
// do not handle type var param and concrete check here
Ok(Ok((unifier.add_ty(TypeEnum::TTuple { ty: vec![] }), false)))
} else if let Some(def_id) = self.pyid_to_def.read().get(&ty_id).cloned() {
// println!("getting def");
Owner

remove println

remove println
sb10q reviewed 2021-11-22 13:07:31 +08:00
@ -114,1 +113,4 @@
} else if let Some(def_id) = self.pyid_to_def.read().get(&ty_id).cloned() {
// println!("getting def");
let def = defs[def_id.0].read();
// println!("got def");
Owner

same

same
sb10q reviewed 2021-11-22 13:09:06 +08:00
@ -69,0 +71,4 @@
assert_eq!(targets.len(), 1, "only support single assignment for now, at {}", targets[0].location);
if let ExprKind::Call { func, args, .. } = &value.node {
if matches!(&func.node, ExprKind::Name { id, .. } if id == &"TypeVar".into()) {
print!("registering typevar {:?}", targets[0].node);
Owner

Remove print. If we want this sort of debug output we should use the logger crate.

Remove print. If we want this sort of debug output we should use the logger crate.
sb10q reviewed 2021-11-22 13:09:26 +08:00
@ -69,0 +87,4 @@
})
.collect::<Vec<_>>();
let res_ty = composer.unifier.get_fresh_var_with_range(&constraints).0;
println!(
Owner

same

same
ychenfo added 1 commit 2021-11-22 15:10:56 +08:00
sb10q reviewed 2021-11-22 17:12:10 +08:00
@ -68,1 +68,4 @@
for stmt in parser_result.into_iter() {
// handle type vars in toplevel
if let StmtKind::Assign { value, targets, .. } = &stmt.node {
assert_eq!(targets.len(), 1, "only support single assignment for now, at {}", targets[0].location);
Owner

Again, is it difficult to iterate?

Again, is it difficult to iterate?
sb10q reviewed 2021-11-22 17:13:02 +08:00
sb10q reviewed 2021-11-22 17:13:14 +08:00
@ -69,0 +88,4 @@
let res_ty = composer.unifier.get_fresh_var_with_range(&constraints).0;
internal_resolver.add_id_type(
if let ExprKind::Name { id, .. } = &targets[0].node { *id } else {
panic!("must assign simple name variable as type variable")
Owner

Do not call panic from errors in user code. Proper error message and Python exceptions are required here.

Do not call panic from errors in user code. Proper error message and Python exceptions are required here.
Author
Collaborator

Thanks for pointing this out, yes this and the previous issue about iterating over assignment should be resolved. Sorry that I forgot to handle these earlier..

Since this is basically the same place in the code as the default default parameter support (in nac3standalone/src/main.rs), maybe I will wait for that one to be mereged first and then modify this PR?

Thanks for pointing this out, yes this and the previous issue about iterating over assignment should be resolved. Sorry that I forgot to handle these earlier.. Since this is basically the same place in the code as the default default parameter support (in `nac3standalone/src/main.rs`), maybe I will wait for that one to be mereged first and then modify this PR?
Owner

Other PR merged.

Other PR merged.
Author
Collaborator

Hi, the new commits resolves the conflict, and support iteration over multiple typevar assignments in the same line. I will also see if my previous PRs introduce the bug newly found..

Hi, the new commits resolves the conflict, and support iteration over multiple typevar assignments in the same line. I will also see if my previous PRs introduce the bug newly found..
ychenfo added 2 commits 2021-11-24 18:42:52 +08:00
Owner

dada7d6688 has nothing to do with this PR. Please keep PRs contained to one topic.

https://git.m-labs.hk/M-Labs/nac3/commit/dada7d668821cafee8e332cbf9096d035f729e23 has nothing to do with this PR. Please keep PRs contained to one topic.
Owner

Also panic in nac3standalone is acceptable, this is just a demonstration program.

Also panic in nac3standalone is acceptable, this is just a demonstration program.
Author
Collaborator

Also panic in nac3standalone is acceptable, this is just a demonstration program.

Ok I see, I will remove the commits about removing panics.

> Also panic in nac3standalone is acceptable, this is just a demonstration program. Ok I see, I will remove the commits about removing panics.
ychenfo force-pushed symbol_resolver_typevar from dada7d6688 to 49240a80ad 2021-11-24 18:53:36 +08:00 Compare
ychenfo added 2 commits 2021-12-01 01:51:14 +08:00
ychenfo changed title from TypeVar, virtual and ForwardRef support in Symbol Resolver to TypeVar and virtual support in Symbol Resolver 2021-12-01 01:56:49 +08:00
sb10q reviewed 2021-12-01 22:44:14 +08:00
sb10q merged commit dfd3548ed2 into master 2021-12-01 22:44:53 +08:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#99
No description provided.