acton icon indicating copy to clipboard operation
acton copied to clipboard

Extend None check to support checking object attributes `if a.b is not None`

Open plajjan opened this issue 2 years ago • 4 comments

Acton Version

0.15.3

Steps to Reproduce

class Foo(object):
    a: ?int
    def __init__(self, a):
        self.a = a

    def do(self):
# NOTE: work around by first assigning a to self.a and use that
#        a = self.a
#        if a is not None:
        if self.a is not None:
            print("YAY")


actor main(env):
    f = Foo(1)
    f.do()
    await async env.exit(0)

Expected Behavior

Program to compile!

Actual Behavior

kll@ThinkYoga:~/terastream/acton$ dist/bin/actonc test/builtins_auto/object-attrs.act
Building file test/builtins_auto/object-attrs.act
  Compiling object-attrs.act for release
   Finished compilation in   0.130 s
  Final compilation step
ERROR: internal compiler error: compilation of generated Zig code failed
NOTE: this is likely a bug in actonc, please report this at:
NOTE: https://github.com/actonlang/acton/issues/new?template=ice.yaml
NOTE: acton 0.16.0.20230708.23.52.42 compiled by ghc 8.10 on linux x86_64
NOTE: cc: 0.11.0-dev.3739+939e4d81e
zig stdout:

zig stderr:
Acton Project Builder
Building in /tmp/actonc-f8fcf8efe2e30e6d
Acton Base Builder
Building in /home/kll/terastream/acton/dist/base
-- filename : rts.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/acton/rts.c
   dir      : /home/kll/terastream/acton/dist/base/out/types/acton
   file_path: /home/kll/terastream/acton/dist/base/out/types/acton/rts.c
-- filename : __builtin__.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/__builtin__.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/__builtin__.c
-- filename : xml.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/xml.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/xml.c
-- filename : time.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/time.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/time.c
-- filename : re.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/re.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/re.c
-- filename : random.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/random.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/random.c
-- filename : process.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/process.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/process.c
-- filename : net.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/net.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/net.c
-- filename : math.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/math.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/math.c
-- filename : numpy.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/numpy.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/numpy.c
-- filename : json.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/json.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/json.c
-- filename : file.c
   full_path: /home/kll/terastream/acton/dist/base/out/types/file.c
   dir      : /home/kll/terastream/acton/dist/base/out/types
   file_path: /home/kll/terastream/acton/dist/base/out/types/file.c
-- filename : object-attrs.c
   full_path: /tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.c
   dir      : /tmp/actonc-f8fcf8efe2e30e6d/out/types
   file_path: /object-attrs.c
-- filename : object-attrs.root.c
   full_path: /tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.root.c
   dir      : /tmp/actonc-f8fcf8efe2e30e6d/out/types
   file_path: /object-attrs.root.c
Building executable from: /tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.root.c -> object-attrs
zig build-lib ActonProject ReleaseFast x86_64-linux-gnu.2.27: error: error(compilation): clang failed with stderr: /tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.c:78:36: error: call to undeclared function '$IdentityOptG_new'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
/tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.c:78:24: warning: cast to 'B_Identity' (aka 'struct B_Identity *') from smaller integer type 'int' [-Wint-to-pointer-cast]
/tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.c:83:36: error: call to undeclared function '$IdentityOptG_new'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
/tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.c:83:24: warning: cast to 'B_Identity' (aka 'struct B_Identity *') from smaller integer type 'int' [-Wint-to-pointer-cast]


zig build-lib ActonProject ReleaseFast x86_64-linux-gnu.2.27: error: the following command failed with 1 compilation errors:
/home/kll/terastream/acton/dist/zig/zig build-lib -cflags -ffile-prefix-map=/tmp/= -- /tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.c -lc -OReleaseFast --cache-dir /tmp/actonc-f8fcf8efe2e30e6d/build-cache --global-cache-dir /home/kll/.cache/acton/build-cache --name ActonProject -static -target x86_64-linux-gnu.2.27 -mcpu x86_64 -I /tmp/actonc-f8fcf8efe2e30e6d -I /home/kll/terastream/acton/dist/base -I /home/kll/terastream/acton/dist/depsout/include --listen=- 
Build Summary: 0/5 steps succeeded; 1 failed (disable with --summary none)
install transitive failure
+- install ActonProject transitive failure
|  +- zig build-lib ActonProject ReleaseFast x86_64-linux-gnu.2.27 1 errors
+- install object-attrs transitive failure
   +- zig build-exe object-attrs ReleaseFast x86_64-linux-gnu.2.27 transitive failure
      +- zig build-lib ActonProject ReleaseFast x86_64-linux-gnu.2.27 (reused)
/tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.c:1:1: error: unable to build C object: clang exited with code 1

the error is:

/tmp/actonc-f8fcf8efe2e30e6d/out/types/object-attrs.c:83:36: error: call to undeclared function '$IdentityOptG_new'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]

plajjan avatar Jul 15 '23 08:07 plajjan

@nordlander is this in the compiler? Or is that part OK but we are simply lacking the necessary C definition for $IdentityOptG_new ?? So @sydow territory?

plajjan avatar Jul 15 '23 08:07 plajjan

I wrote some benchmarking programs, see https://github.com/hanabi1224/Programming-Language-Benchmarks/pull/392, and all the code looks quite horrible because I have to assign local variables from object attributes before I can use them. It's a bit of a show case of Acton so would be nice to address this :)

plajjan avatar Jul 15 '23 08:07 plajjan

This is in part a simple omission bug (builtin witness IdentityOpt isn't properly defined). But more importantly, it also illustrates the limitation of our current approach to type inference for optionals!

Recall that if x is an optional int, when we write

if x is not None:
    ... x ...

we also expect that x can be treated as a proper int in the scope indicated by .... This is also what the compiler does, by explicitly looking out for variable-is-not-none expressions and locally redefining the tested variable to its non-optional type inside the corresponding scope.

But if the tested expression is just slightly more complicated, as in

if self.x is not None:
   ... self.x ...

the same approach isn't as clear-cut. Now we'd have to locally redefine not self but the class of self to achieve the same effect. And such a redefinition would risk affecting other objects in scope as well, incorrectly giving all their attributes x a non-optional type! Moreover, even if we somehow could work around this particular case, we'd just have to replace self.x with self.other.y to see the problem reappear on yet another level. For this reason, the compiler currently stops at single variables when it searches for narrowing tests.

So clearly the current approach is not ideal, perhaps it should be considered more of a hack. The question is what to do about it in the long run. I can see three partially overlapping solutions:

  1. Adjust the type-checker so that it maintains a set of narrowed expressions in addition to its regular variable environment. This would easily support more complex forms of tests, as the narrowing information would be bound to the tested expressions themselves rather than their contained variables. Care would nevertheless have to be taken in the presence of side-effects, however, so some complexity line would still have to be drawn somewhere (if not as tight as it currently is).
  2. Switch to the new integrated class & actor syntax. This would remove the need to qualify attributes via an explicit self, which perhaps covers most of the desired test forms anyway.
  3. Implement Python's pattern-matching feature. This would allow arbitrary narrowing tests to be combined with explicit name capturing, enabling constructs like the follwing:
match very_complex_expr_of_optional_type():
    case int as x:
        ... x ...

Solutions 2 and 3 are in the pipeline already, so maybe we should defer investigating alternative 1 until we're sure we still need it in the presence of the others.

Adding the missing witness IdentityOpt would be trivial but must be redone once we start supporting general unions. Most importantly, doing this would not help the referenced hanabi benchmark since it crucially relies on type narrowing, not just correct tests. But the new class syntax (alt. 2) would, so maybe that should be our top priority.

nordlander avatar Jul 20 '23 11:07 nordlander

I think 2 will definitely reduce this problem in practice by quite a lot. However, for many of these cases I don't think 3 is a very natural solution so I suspect that we will need at least a partial 1, but like you said, let's hold off until we have 2 in place before discussing this further.

plajjan avatar Jul 24 '23 22:07 plajjan

After writing lots more Acton code I think solution 1 as listed by @nordlander above is the most idiomatic solution to this and I have created #1906 as the spiritual successor of this issue for tracking that work. We also have #834 for addition of pattern matching and #1044 for unification of the actor / class syntax. Closing this.

plajjan avatar Sep 09 '24 08:09 plajjan