compiler: allows string annotation

[pca006132]

ARTIQ Pull Request

Description of Changes

According to PEP484, type hint can be a string literal for forward references. With PEP563, type hint would be preserved in annotations in string form.

Related Issue

Type of Changes

	Type
✓	🐛 Bug fix

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

• Use correct spelling and grammar.

Code Changes

• Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.
• Test your changes or have someone test them. Mention what was tested and how.
• Check, test, and update the unittests in /artiq/test/ or gateware simulations in /artiq/gateware/test

Test cases:

from artiq.experiment import *

class ArtiqTest(EnvExperiment):

    def build(self):
        self.setattr_device('core')

    @kernel
    def run(self):
        self.foo(1)

    @kernel
    def foo(self, v: 'TInt32'):
        self.core.reset()
        print('foo=', v)

The original version would fail with error message

test.py:14:1: error: type annotation for argument 'v', ''TInt32'', is not an ARTIQ type
def foo(self, v: 'TInt32'):
^                          
test.py:14:1: note: in kernel function here
def foo(self, v: 'TInt32'):
^                          
test.py:11:9-11:17: note: while inferring a type for an attribute 'foo' of a host object
        self.foo(1)
        ^^^^^^^^

while the fixed version would pass successfully run the code.

Git Logistics

• Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
• Write short & meaningful commit messages. Review each commit for messages (git show). Format:

  topic: description. < 50 characters total.
  
  Longer description. < 70 characters per line
  

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

[dnadlinger]

(Incidentally, this is the issue I encountered when trying 3.9 a while back.)

[lriesebos]

@sbourdeauducq do you think this could be backported to ARTIQ 6? probably also #1705 then.

[sbourdeauducq]

Why not use 7 if you need this?
You can also try from __future__ import annotations which might solve your initial problem (I haven't tried if the legacy compiler accepts it).

[lriesebos]

For our running systems, we stick with the latest release version of ARTIQ, currently 6.

The issue fixed by this and the other PR is actually caused by using from __future__ import annotations. See https://git.m-labs.hk/M-Labs/nac3-spec/issues/19 for the original issue. I guess it could be considered a bugfix.

If you do not think this should be backported, then I will solve it by removing the future annotations import in my file and use string type annotations where postponed evaluation of annotations is needed. The ARTIQ types obviously do not need postponed evaluation.

[sbourdeauducq]

FYI even Python 3.11 does not implement the new behavior:

Python 3.11.0a4 (main, Jan 13 2022, 19:39:06) [GCC 10.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class X:
...   x: int
... 
>>> X.__annotations__
{'x': <class 'int'>}
>>> def foo(x: int):
...   pass
... 
>>> foo.__annotations__
{'x': <class 'int'>}

NAC3 does depend on use of from __future__ import annotations for postponed evaluation on the other hand; string annotations are not going to be supported.

Have you tested the release-6 compiler with those changes and are you able to tell that this would not cause regressions?

[lriesebos]

I agree that string annotations would just be a workaround if postponed evaluation is not supported for ARTIQ 6. Supporting postponed evaluation is the preferred way to go.

I have cherry picked c29a149 and f531af5 on top of the release 6 branch, and it does fix the issue. All non-ARTIQ_ROOT tests pass and artiq.test.coredevice.test_embedding also passes when setting an ARTIQ_ROOT. So at this moment, no regressions were found, but I can not run all tests here at this moment.

I will leave it up to you if you want to backport this or not. Even if string annotations are not preferred, they can mitigate the issue for now and we can revert the changes to our experiments when ARTIQ 7 is released and we move our systems to the new release.