Add dict to stdlib

[dpdani]

No description provided.

[antocuni]

Thank you a lot for this PR, I love it.

I especially appreciate that you highlighted all the various "pain point": I agree we should do better, and I opened an issue for each of them (some are easy to fix/implement, others are harder).

[dpdani]

@antocuni I left a couple more [Antonio] for you 👀

[antocuni]

I think it says that someone tried to do x % 0 (not that it might do modulo 0... someone actually DID that).
You get the same tb with this toy program:

def main() -> void:
    print(2 % 0)

We should detect the case and raise a proper exception/panic, see #183.

That said, I cannot really understand where it comes from, because I don't see any modulo operator in your source code, and I cannot reproduce it.
Can you got this error with a different/previous version of the code?
Or, can you show me how to reproduce?

[dpdani]

Sure! If you substitute & → % on line 74 you should see that error

[antocuni]

@dpdani here are my updated comments

[antocuni]

@dpdani it seems I need to manually approve the workflow to run the tests every time you do a new push. I'll try to invite you as a maintainer, hopefully it will help.

[dpdani]

joined 👍

[dpdani]

The test passes 🎉

[antocuni]

@dpdani I have some news which should be relevant to you:

1. I merged Add support for array of structs #195: this should make it possible to use an array of Entry as you wanted to do
2. I marged introduce the stdlib directory #187: it officially introduces the stdlib directory and you can now seamlessly import modules from it
3. I have a WIP Introduce stdlib/array.spy #196: I think the most interesting bit for you is to see how I write tests for array.spy, which you could use as inspiration for testing dict.spy. As you can see currently everything is very explicit, although eventually I would like to have something more straightforward, so that people don't have to call self.compile() manually... but I suppose that for now it's good enough

[dpdani]

Hi @antocuni 👋

Nice!! That's a lot of good improvements!

I have been focused on other things for a while and I'll probably get back to spy dict in a week or so 👍

[dpdani]

And maybe someday we will roll our own spytest? 👀

[dpdani]

hey there @antocuni 👋

big merge from main coming through 🚂
along with tests (shamelessly written by Junie) and array of Entry

I'm seeing this error in all doppler and C tests, but not in the interpreter tests. How do I deal with it?

E       spy.errors.SPyError: TypeError: type `unsafe::ptr[dict::dict[i32, i32]::DictData]` does not support assignment to attribute 'data'
E          --> /Users/dp/repos/spy/stdlib/dict.spy:161:9
E       161 |         self.__ll__.data = new
E           |         |_________| this is `unsafe::ptr[dict::dict[i32, i32]::DictData]`
E       
E          --> /Users/dp/repos/spy/stdlib/dict.spy:161:9
E       161 |         self.__ll__.data = new
E           |         |____________________| operator::SETATTR called here

[antocuni]

hey there @antocuni 👋

big merge from main coming through 🚂 along with tests (shamelessly written by Junie) and array of Entry

I'm seeing this error in all doppler and C tests, but not in the interpreter tests. How do I deal with it?

E       spy.errors.SPyError: TypeError: type `unsafe::ptr[dict::dict[i32, i32]::DictData]` does not support assignment to attribute 'data'
E          --> /Users/dp/repos/spy/stdlib/dict.spy:161:9
E       161 |         self.__ll__.data = new
E           |         |_________| this is `unsafe::ptr[dict::dict[i32, i32]::DictData]`
E       
E          --> /Users/dp/repos/spy/stdlib/dict.spy:161:9
E       161 |         self.__ll__.data = new
E           |         |____________________| operator::SETATTR called here

@dpdani
this is actually SPy's static typing doing its job :)

In the method resize, self.__ll__ is of type DictData, which does not have a data field:

    @struct
    class DictData:
        index: ptr[i32]
        log_size: i32  # capacity
        length: i32  # number of items stored
        entries: ptr[Entry]

So, the error you get it's actually correct.

Why the [interp] tests pass then? Because in interp mode, type errors are raised only if/when we actually try to execute that line (like in Python): evidently the method resize is never called by any test.

However, when we do redshift we check ALL functions for TypeErrors, and that's why compilation fails. This is called "eager error mode", and it's the default (because it's what you expect in a statically typed language, after all).

There is also the "lazy error mode", in which TypeErrors don't cause compilation to fail, but they are translated into raise statements.

E.g., consider this program:

def main() -> None:
    print('hello')
    if 0:
        1 + 'xxx'
    print('world')

❯ spy /tmp/x.spy 
hello
world

❯ spy --redshift /tmp/x.spy    # eager errors
TypeError: cannot do `i32` + `str`
   --> /tmp/x.spy:4:9
  4 |         1 + 'xxx'
    |         ^ this is `i32`

   --> /tmp/x.spy:4:13
  4 |         1 + 'xxx'
    |             |___| this is `str`

   --> /tmp/x.spy:4:9
  4 |         1 + 'xxx'
    |         |_______| operator::ADD called here


❯ spy --redshift --error-mode=lazy /tmp/x.spy 

def main() -> None:
    print_str('hello')
    if `operator::i32_to_bool`(0):
        raise TypeError('cannot do `i32` + `str`') # /.../astframe.py:583
    print_str('world')

In tests, you can turn on lazy errors by using self.compile(src, error_mode='lazy'), and they pass.
However, the right thing to do is to fix resize, of course :)

[dpdani]

oh yeah, big brain fart yesterday 😆

[antocuni]

oh yeah, big brain fart yesterday 😆

actually, it took a while also for me to understand what was happening.
I think the lesson is that we could improve the error message :).
I opened this issue to discuss how: #239

[dpdani]

Agreed 👍

I think I want to tackle deletes and the iterator in this PR, and the rest can be separated.
What do you think?

[antocuni]

I think I want to tackle deletes and the iterator in this PR, and the rest can be separated. What do you think?

iterators are possible, but ATM I don't have any language support for del and __delitem__, so those are better to be put in their own PR

[dpdani]

I'm happy with the state of this PR.

@antocuni want to do a review?

[antocuni]

Thanks @dpdani.
I didn't review deeply the actual hashtable strategy but I assume it's correct thanks to tests.
I'm curious to know whether the basic idea is similar to CPython's dict or this is a different strategy.
Maybe if you feel like you could add some short comment at the beginning of dict.spy to explain it?

Also, I'd suggest to rename it to _dict.spy, for consistency with _range.spy and _list.spy.
Eventually I will need a way to automatically make these names available in the builtins module, but currently I cannot do it easily because of bootstrapping issues.

[antocuni]

thanks to #240 we now have assert

[dpdani]

but the chained comparison doesn't work, and using and doesn't work either:

self = <spy.parser.Parser object at 0x106fbcf50>
primary = 'not implemented yet: BoolOp', secondary = 'this is not supported'
loc = <Loc: '/Users/dp/repos/spy/stdlib/_dict.spy 37:15 37:68'>

    def error(self, primary: str, secondary: str, loc: Loc) -> NoReturn:
>       raise SPyError.simple("W_ParseError", primary, secondary, loc)
E       spy.errors.SPyError: ParseError: not implemented yet: BoolOp
E          --> /Users/dp/repos/spy/stdlib/_dict.spy:37:16
E        37 |         assert MIN_LOG_SIZE <= log_size and log_size <= MAX_LOG_SIZE
E           |                |___________________________________________________| this is not supported

I'll split it into two asserts

[antocuni]

oh right, I always forget about missing features.
I opened #248 and #249 so we don't forget.
Feel free to open issues whenever you find missing features.

[antocuni]

you can add KeyError here:

spy/spy/vm/exc.py

Lines 127 to 174 in ead6d95

	@BUILTINS.builtin_type('TypeError')
	class W_TypeError(W_StaticError):
	"""
	Note that TypeError is a subclass of StaticError
	"""
	pass
	@BUILTINS.builtin_type('ValueError')
	class W_ValueError(W_Exception):
	pass
	@BUILTINS.builtin_type('IndexError')
	class W_IndexError(W_Exception):
	pass
	@BUILTINS.builtin_type('ParseError')
	class W_ParseError(W_Exception):
	pass
	@BUILTINS.builtin_type('ImportError')
	class W_ImportError(W_Exception):
	pass
	@BUILTINS.builtin_type('ScopeError')
	class W_ScopeError(W_Exception):
	pass
	@BUILTINS.builtin_type('NameError')
	class W_NameError(W_Exception):
	pass
	@BUILTINS.builtin_type('PanicError')
	class W_PanicError(W_Exception):
	pass
	@BUILTINS.builtin_type('ZeroDivisionError')
	class W_ZeroDivisionError(W_Exception):
	pass
	@BUILTINS.builtin_type('AssertionError')
	class W_AssertionError(W_Exception):
	pass
	@BUILTINS.builtin_type('WIP')
	class W_WIP(W_Exception):
	"""
	Raised when something is supposed to work but has not been implemented yet
	"""

[dpdani]

I don't quite see a way around this error 🤔
I guess, I'll just leave it as raise KeyError() without an argument?

E               spy.errors.SPyError: TypeError: cannot call blue function with red arguments
E                  --> /Users/dp/repos/spy/stdlib/_dict.spy:161:28
E               161 |             raise KeyError(str(key))
E                   |                            |______| this is red
E               
E                  --> /Users/dp/repos/spy/spy/vm/exc.py:67:1
E                67 |         def w_new(vm: 'SPyVM', w_cls: W_Type, w_message: W_Str) -> T:
E                   | |___________________________________________________________________| function defined here
E               
E                  --> /Users/dp/repos/spy/stdlib/_dict.spy:161:19
E               161 |             raise KeyError(str(key))
E                   |                   |________________| operator::CALL called here

[dpdani]

By which I mean raise KeyError("") with an empty string because an argument is required

[antocuni]

yes, current support for exceptions is VERY limited:

1. raise always causes a panic and exceptions cannot be caught
2. the error message must be blue (this is the error that you are seeing).

Note that you don't need raise KeyError(""). You can just do raise KeyError

[antocuni]

because an argument is required

why required?
The following example works for me:

def main() -> None:
    raise KeyError

❯ spy /tmp/x.spy 
KeyError: 
   --> /tmp/x.spy:2:2
  2 |     raise KeyError
    |  |_______________|

[dpdani]

mm right, but if I write raise KeyError() with the parens then it doesn't work.

[antocuni]

right, opened #250 for it

[antocuni]

I think this is good enough for now. However, eventually we probably want to introduce the same check as in CPython and detect whether the dict has changed size during the iteration.
So maybe we can add a # TODO comment?

[antocuni]

we have a better way to deal with hash nowadays 🎉.
Look how we implement len:

spy/spy/vm/modules/builtins.py

Lines 112 to 125 in ead6d95

	@BUILTINS.builtin_func(color='blue', kind='metafunc')
	def w_len(vm: 'SPyVM', wam_obj: W_MetaArg) -> W_OpSpec:
	w_T = wam_obj.w_static_T
	if w_fn := w_T.lookup_func('__len__'):
	w_opspec = vm.fast_metacall(w_fn, [wam_obj])
	return w_opspec
	t = w_T.fqn.human_name
	raise SPyError.simple(
	'W_TypeError',
	f'cannot call len(`{t}`)',
	f'this is `{t}`',
	wam_obj.loc
	)

We should do the same for hash.
We can do it in this PR, but I'm also happy to do it in a follow-up PR if you prefer.

[dpdani]

I'll leave the hash rework and the design comment to tomorrow

[antocuni]

LGTM, merging it.
We finally have a dict type 🎉.
Thank you, this is a big step for SPy!

[antocuni]

this is good for now, but ideally I'd like these functions to be in W_I8.w_hash etc, similarly to what we do for __str__, __repr__ and __len__.
It's not a merge blocker though, we can do it in a follow up PR