M-Labs
/
nac3
8
0
Fork 5
Code Issues 87 Pull Requests 3 Releases Wiki Activity

Constant Default Parameter Support #98

Merged
sb10q merged 7 commits from default_parameter into master 2021-11-23 07:32:09 +08:00
Conversation 3 Commits 7 Files Changed 10
ychenfo commented 2021-11-13 16:53:52 +08:00
Collaborator

Add support for constant default parameter

Add support for constant default parameter
sb10q requested review from pca006132 2021-11-19 12:58:56 +08:00
pca006132 reviewed 2021-11-19 15:38:48 +08:00
nac3core/src/toplevel/helper.rs Outdated
@ -344,0 +354,4 @@
if let Ok(v) = v.try_into() {
Ok(SymbolValue::I32(v))
} else {
Ok(SymbolValue::I64(v.try_into().unwrap()))
pca006132 commented 2021-11-19 15:38:48 +08:00

Should we just accept int32? int64 is handled in the call part, e.g. int64(1234).

Should we just accept int32? int64 is handled in the call part, e.g. `int64(1234)`.
ychenfo commented 2021-11-20 05:00:31 +08:00
Poster
Collaborator

this is handled in the new commit below

this is handled in the new commit below
ychenfo marked this conversation as resolved
pca006132 commented 2021-11-19 15:40:00 +08:00

LGTM, but we should probably add tests to nac3artiq/nac3standalone later.

LGTM, but we should probably add tests to nac3artiq/nac3standalone later.
ychenfo commented 2021-11-20 05:02:09 +08:00
Poster
Collaborator

LGTM, but we should probably add tests to nac3artiq/nac3standalone later.

Yes, some demo python files related to this will be added later.

> LGTM, but we should probably add tests to nac3artiq/nac3standalone later. Yes, some demo python files related to this will be added later.
sb10q reviewed 2021-11-20 13:52:27 +08:00
nac3core/src/toplevel/helper.rs Outdated
@ -344,0 +355,4 @@
Ok(SymbolValue::I32(v))
} else {
Err(format!(
"int64 default parameter should be specified explicitly by `int64()` at {}",
sb10q commented 2021-11-20 13:52:22 +08:00
Owner

Isn't that an internal compiler error if this code is executed, not a problem with the user code?
Using unreachable!() sounds more appropriate here.

Isn't that an internal compiler error if this code is executed, not a problem with the user code? Using ``unreachable!()`` sounds more appropriate here.
ychenfo marked this conversation as resolved
sb10q reviewed 2021-11-20 13:52:32 +08:00
nac3core/src/toplevel/helper.rs Outdated
@ -344,0 +355,4 @@
Ok(SymbolValue::I32(v))
} else {
Err(format!(
"int64 default parameter should be specified explicitly by `int64()` at {}",
sb10q commented 2021-11-20 13:52:32 +08:00
Owner

Isn't that an internal compiler error if this code is executed, not a problem with the user code?
Using unreachable!() sounds more appropriate here.

Isn't that an internal compiler error if this code is executed, not a problem with the user code? Using ``unreachable!()`` sounds more appropriate here.
pca006132 commented 2021-11-20 14:39:07 +08:00

No I don't think so? It is possible for users to specify some default value outside the range of int64. I think a more appropriate error message would be value out of range and only say int64 if the value is really an int64?

No I don't think so? It is possible for users to specify some default value outside the range of int64. I think a more appropriate error message would be value out of range and only say `int64` if the value is really an int64?
sb10q commented 2021-11-20 14:39:43 +08:00
Owner

OK, yes.

OK, yes.
ychenfo commented 2021-11-21 22:06:50 +08:00
Poster
Collaborator

Thanks, this error message is updated in the new commit

Thanks, this error message is updated in the new commit
ychenfo marked this conversation as resolved
sb10q reviewed 2021-11-20 13:54:08 +08:00
nac3core/src/toplevel/helper.rs Outdated
@ -344,0 +390,4 @@
.iter()
.map(|x| Self::parse_parameter_default_value(x))
.collect::<Result<Vec<_>, _>>()?
)),
sb10q commented 2021-11-20 13:54:08 +08:00
Owner

We also want to support globals defined in the module, e.g.:

FOO = 1234

class XXX:
   def yyy(self, z: int32 = FOO):
      ...

See the ARTIQ drivers, this is used in many places.

We also want to support globals defined in the module, e.g.: ```python FOO = 1234 class XXX: def yyy(self, z: int32 = FOO): ... ``` See the ARTIQ drivers, this is used in many places.
sb10q commented 2021-11-20 14:02:39 +08:00
Owner

In fact, why not just compile the expression like any other?
Things like def foo(x=[i*2 for i in range(3)]) are also valid Python.

In fact, why not just compile the expression like any other? Things like ``def foo(x=[i*2 for i in range(3)])`` are also valid Python.
pca006132 commented 2021-11-20 14:43:17 +08:00

The expression should be immutable and primitive types/tuples. I think we can eval() the expression and set the default value if its type can be accepted.

The expression should be immutable and primitive types/tuples. I think we can `eval()` the expression and set the default value if its type can be accepted.
sb10q commented 2021-11-20 14:52:45 +08:00
Owner

eval is slow and not available in nac3standalone.

``eval`` is slow and not available in nac3standalone.
pca006132 commented 2021-11-20 14:56:27 +08:00

Regarding performance issue, I think we can optimize it later if it is really that slow. For nac3standalone, I think we can support simple cases first and leave this later, I don't think this is of high priority?

Regarding performance issue, I think we can optimize it later if it is really that slow. For nac3standalone, I think we can support simple cases first and leave this later, I don't think this is of high priority?
sb10q commented 2021-11-20 15:00:05 +08:00
Owner

Well the only thing we really need is module globals. If that can be done then something like the current code seems fine.

Well the only thing we really need is module globals. If that can be done then something like the current code seems fine.
ychenfo commented 2021-11-20 16:13:59 +08:00
Poster
Collaborator

Ok I will add module globals support on top of the current constant support

Ok I will add module globals support on top of the current constant support
ychenfo commented 2021-11-21 06:03:17 +08:00
Poster
Collaborator

Sorry, still one thing not very clear about this...

To handle complex expressions(calling eval for now? Also, expressions like Module.T which refers to things defined in another module imported) as default value, should we add a function in symbol resolver, which accepts a ast::Expr and return a SymbolValue?

Sorry, still one thing not very clear about this... To handle complex expressions(calling `eval` for now? Also, expressions like `Module.T` which refers to things defined in another module imported) as default value, should we add a function in symbol resolver, which accepts a `ast::Expr` and return a `SymbolValue`?
sb10q commented 2021-11-21 11:47:18 +08:00
Owner

Let's not call eval and let's keep the more restricted approach. Keep the current mechanism and simply add support for being able to use globals e.g.:

FOO = 1234

class XXX:
   def yyy(self, z: int32 = FOO):
      ...
Let's not call ``eval`` and let's keep the more restricted approach. Keep the current mechanism and simply add support for being able to use globals e.g.: ```python FOO = 1234 class XXX: def yyy(self, z: int32 = FOO): ... ```
ychenfo commented 2021-11-21 22:18:48 +08:00
Poster
Collaborator

I have added the basic module primitive/tuple global default parameter in the new commit.

No eval is used, only simple variable name is supported, but still involve some changes in symbol resolver(adding a function in symbol resolver which accepts a ast::Expr and return a SymbolValue). If I do not miss anything, I do not think there is another way to workaround this without changing symbol resolver... since in nac3artiq the core compiler does not know anything about the module globals if symbol resolver does not provide any information to it. Please have a review, thanks!

I have added the basic module primitive/tuple global default parameter in the new commit. No `eval` is used, only simple variable name is supported, but still involve some changes in symbol resolver(adding a function in symbol resolver which accepts a `ast::Expr` and return a `SymbolValue`). If I do not miss anything, I do not think there is another way to workaround this without changing symbol resolver... since in nac3artiq the core compiler does not know anything about the module globals if symbol resolver does not provide any information to it. Please have a review, thanks!
sb10q commented 2021-11-22 12:48:39 +08:00
Owner

LGTM

LGTM
sb10q reviewed 2021-11-20 13:55:15 +08:00
nac3core/src/toplevel/helper.rs Outdated
@ -344,0 +386,4 @@
Err(format!("only allow constant integer here at {}", default.location))
}
}
ast::ExprKind::Tuple { elts, .. } => Ok(SymbolValue::Tuple(elts
sb10q commented 2021-11-20 13:55:15 +08:00
Owner

Lists should also be supported (also if they are a module global, see comment below).

Lists should also be supported (also if they are a module global, see comment below).
pca006132 commented 2021-11-20 14:44:14 +08:00

Lists are not immutable, I think we discussed this previously?

M-Labs/nac3-spec#6

Function parameter defaults for kernel, portable, and rpc functions are very important for us. The limitation of only allowing immutable or primitive default types is fine.

Lists are not immutable, I think we discussed this previously? https://git.m-labs.hk/M-Labs/nac3-spec/issues/6 > Function parameter defaults for kernel, portable, and rpc functions are very important for us. The limitation of only allowing immutable or primitive default types is fine.
sb10q commented 2021-11-20 14:46:25 +08:00
Owner

Indeed, and that's a well-known pitfall in CPython as well.
ARTIQ drivers do use lists with default parameter. AFAICT this can be changed to tuple, so far...

Indeed, and that's a well-known pitfall in CPython as well. ARTIQ drivers do use lists with default parameter. AFAICT this can be changed to tuple, so far...
sb10q commented 2021-11-20 14:49:03 +08:00
Owner

Well we can start with tuples only for now and see if that causes any serious problems.

Well we can start with tuples only for now and see if that causes any serious problems.
sb10q reviewed 2021-11-20 13:57:46 +08:00
nac3core/src/toplevel/helper.rs Outdated
@ -344,0 +372,4 @@
ast::ExprKind::Constant { value, .. } => handle_constant(value, &default.location),
ast::ExprKind::Call { func, args, .. } if {
match &func.node {
ast::ExprKind::Name { id, .. } => *id == "int64".into(),
sb10q commented 2021-11-20 13:57:46 +08:00
Owner

Aren't we matching on id(numpy.int64) elsewhere?
https://git.m-labs.hk/M-Labs/nac3/src/branch/master/nac3artiq/src/lib.rs#L270-L306
I'm fine with string matches since they're simpler and potentially faster, but we should do things consistently.

Aren't we matching on ``id(numpy.int64)`` elsewhere? https://git.m-labs.hk/M-Labs/nac3/src/branch/master/nac3artiq/src/lib.rs#L270-L306 I'm fine with string matches since they're simpler and potentially faster, but we should do things consistently.
ychenfo commented 2021-11-20 16:12:31 +08:00
Poster
Collaborator

I think here we are handling default parameters in nac3core so we do not really know the id(numpy.int64)?

In type inference module we also do things using strings matches here and here. So I think it is ok to use strings? Or did I miss anything?

I think here we are handling default parameters in nac3core so we do not really know the `id(numpy.int64)`? In type inference module we also do things using strings matches [here](https://git.m-labs.hk/M-Labs/nac3/src/branch/master/nac3core/src/typecheck/type_inferencer/mod.rs#L659) and [here](https://git.m-labs.hk/M-Labs/nac3/src/branch/master/nac3core/src/typecheck/type_inferencer/mod.rs#L624). So I think it is ok to use strings? Or did I miss anything?
pca006132 commented 2021-11-20 18:19:33 +08:00

Aren't we matching on id(numpy.int64) elsewhere?
https://git.m-labs.hk/M-Labs/nac3/src/branch/master/nac3artiq/src/lib.rs#L270-L306
I'm fine with string matches since they're simpler and potentially faster, but we should do things consistently.

That is matching on the value type, not about functions.

> Aren't we matching on `id(numpy.int64)` elsewhere? https://git.m-labs.hk/M-Labs/nac3/src/branch/master/nac3artiq/src/lib.rs#L270-L306 I'm fine with string matches since they're simpler and potentially faster, but we should do things consistently. That is matching on the value type, not about functions.
sb10q commented 2021-11-21 11:29:39 +08:00
Owner

What's the difference? There isn't one in CPython.
Also this can cause issues e.g. if the user types from numpy import int64 as i64. CPython and nac3artiq will expect i64, whereas nac3core will expect int64.
If that's the simplest thing to do, I'm fine having restrictions such as "numpy types must be imported as their reserved NAC3 keywords int64/int32/..." but they should be documented and enforced by the compiler.

What's the difference? There isn't one in CPython. Also this can cause issues e.g. if the user types ``from numpy import int64 as i64``. CPython and nac3artiq will expect ``i64``, whereas nac3core will expect ``int64``. If that's the simplest thing to do, I'm fine having restrictions such as "numpy types must be imported as their reserved NAC3 keywords int64/int32/..." but they should be documented and enforced by the compiler.
sb10q commented 2021-11-21 11:35:27 +08:00
Owner

Anyway this isn't the most pressing issue at the moment and we can do this inconsistent string/id match for now. Let's continue this discussion in #105

Anyway this isn't the most pressing issue at the moment and we can do this inconsistent string/id match for now. Let's continue this discussion in https://git.m-labs.hk/M-Labs/nac3/issues/105
ychenfo force-pushed default_parameter from 599aeb7bb3 to e75ec35687 2021-11-21 05:53:35 +08:00 Compare
sb10q reviewed 2021-11-21 11:41:22 +08:00
nac3core/src/toplevel/composer.rs Outdated
@ -1322,0 +1350,4 @@
default_value: match default {
None => None,
Some(default) => {
if name == "self".into() {
sb10q commented 2021-11-21 11:41:22 +08:00
Owner

Technically CPython lets you name self whatever you want, it's only a convention.
So this test should be "the function is a method and we are looking at its first parameter".

Technically CPython lets you name ``self`` whatever you want, it's only a convention. So this test should be "the function is a method and we are looking at its first parameter".
ychenfo commented 2021-11-21 22:32:52 +08:00
Poster
Collaborator

Would it be acceptable that in nac3, we just enforce and document that the self should always be self and should not be named arbitrarily? As I think this would not cause very big problems and our existing nac3 codebase is already doing this (e.g., here, here, and here)

Would it be acceptable that in nac3, we just enforce and document that the `self` should always be `self` and should not be named arbitrarily? As I think this would not cause very big problems and our existing nac3 codebase is already doing this (e.g., [here](https://git.m-labs.hk/M-Labs/nac3/src/branch/master/nac3core/src/codegen/expr.rs#L293), [here](https://git.m-labs.hk/M-Labs/nac3/src/branch/master/nac3core/src/codegen/expr.rs#L348), and [here](https://github.com/m-labs/artiq/blob/nac3/artiq/language/core.py#L56))
sb10q commented 2021-11-22 11:13:04 +08:00
Owner

It is acceptable but not super clean - is it difficult to use the condition I proposed instead?

https://github.com/m-labs/artiq/blob/nac3/artiq/language/core.py#L56 is unavoidable without a lot of code (AFAICT) because CPython does not tell you if something is a method until it is bound, but inside NAC3 we have more information.

It is acceptable but not super clean - is it difficult to use the condition I proposed instead? https://github.com/m-labs/artiq/blob/nac3/artiq/language/core.py#L56 is unavoidable without a lot of code (AFAICT) because CPython does not tell you if something is a method until it is bound, but inside NAC3 we have more information.
ychenfo added 1 commit 2021-11-21 22:02:14 +08:00
ychenfo force-pushed default_parameter from 59bdb1be50 to ee7c1eb11d 2021-11-21 22:07:53 +08:00 Compare
sb10q reviewed 2021-11-22 13:14:35 +08:00
nac3standalone/src/main.rs Outdated
@ -69,0 +74,4 @@
internal_resolver.add_module_global(*id, value);
}
} else {
unimplemented!("only single assign supported now")
sb10q commented 2021-11-22 13:14:35 +08:00
Owner

Come on. It's not more code to iterate on targets, is it?

Come on. It's not more code to iterate on ``targets``, is it?
ychenfo commented 2021-11-22 14:18:55 +08:00
Poster
Collaborator

Oh sure, now the symbol resolver change is ok, I will add this support now.

Oh sure, now the symbol resolver change is ok, I will add this support now.
ychenfo commented 2021-11-23 01:05:45 +08:00
Poster
Collaborator

This is supported in the latest commit, as well as a better type check for default parameter, please have a review, thanks!

This is supported in the latest commit, as well as a better type check for default parameter, please have a review, thanks!
ychenfo added 1 commit 2021-11-23 01:03:20 +08:00
ychenfo force-pushed default_parameter from 5097e9e15a to 03cdca606d 2021-11-23 01:43:13 +08:00 Compare
sb10q merged commit 4587088835 into master 2021-11-23 07:32:09 +08:00
sb10q commented 2021-11-23 07:32:25 +08:00
Owner

Thanks. @ychenfo please add tests

Thanks. @ychenfo please add tests
Sign in to join this conversation.
Reviewers
No reviewers
pca006132
Labels
Clear labels   
UROP0622
   
future-extension
   
high-priority
   
low-priority
No Label
UROP0622
future-extension
high-priority
low-priority
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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#98
There is no content yet.
Delete Branch "default_parameter"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?