arturo-lang / arturo

Simple, expressive & portable programming language for efficient scripting
http://arturo-lang.io
MIT License
721 stars 32 forks source link

[General] All `SetInPlace` or `InPlace=` that change the type of object should b... #786

Open github-actions[bot] opened 2 years ago

github-actions[bot] commented 2 years ago

[General] All SetInPlace or InPlace= that change the type of object should be changed

It doesn't work when in-place changing passed parameters to a function

The above is mostly a hack to get around this

https://github.com/arturo-lang/arturo/blob/cdc7fd7a9d33d5925a4cb9931c68ea84c6935dd4/src/library/Collections.nim#L999

                        SetInPlace(newBlock(InPlaced.a.removeFirst(y)))
                    elif (hadAttr("index")):
                        # TODO(General) All `SetInPlace` or `InPlace=` that change the type of object should be changed
                        #  It doesn't work when in-place changing passed parameters to a function
                        #  The above is mostly a hack to get around this
                        #  labels: bug, critical, vm
                        InPlaced.kind = Block
                        InPlaced.a = InPlaced.a.removeByIndex(y.i)
                        #SetInPlace(newBlock(InPlaced.a.removeByIndex(y.i)))
                    else:
                        SetInPlace(newBlock(InPlaced.a.removeAll(y)))
                elif InPlaced.kind == Dictionary:
ndex db8d223d8a..2b81539c8c 100644
++ b/src/library/Converters.nim

76896744e97fc7dd1dbd1223ab5926d0fc54a525

RickBarretto commented 1 year ago

I have a question. Should be illegal change the type when I'm using InPlace modification? I'm asking because I don't want to commit errors doing it for future commits/PR/etc...

RickBarretto commented 1 year ago

Also, I have an idea (IDK if it would work, but if yes, it would make it easy to fix this behavior without errors).

So, my plan is statically check (at compile-time) for SetInplace and Inplace = if the current type is equal to the new type.

Generic example

proc SetInplace(a: untyped) =
  static:
    if inp.kind == a.kind:
     panic()...

@drkameleon what do you think? I would be happy trying to do it... 🙂

drkameleon commented 1 year ago

Hmm, hmm... I think that's a very tricky one that I'm very afraid to even touch myself (that's why the issue has been stuck as pending for so long).

Basically, there are many, many case where this sort of thing is happening.

E.g.

1 + 2.0 -> takes an Integer and adds a Floating, so the result is? Floating

Now if we do it with in-place modification (and x being an Integer):

'x + 2.0 -> again, X becomes a Floating

So, there is no problem at all with changing the kind of the InPlace object. We actually want this type of transformations to happen. The point is how we do it.

Currently, the whole thing works rather fine. The problem is there is a corner case (passed literals) where it doesn't. First of all, we should isolate that case. And then see how to go about it... (I'll try to reproduce what I - at least - considered an "error" when creating this issue)

But in any case: this is not simple at all 😉

RickBarretto commented 1 year ago

But in any case: this is not simple at all 😉

I got it! Yeah, this issue will take some time to be solved... Revisit some parts would be interesting, opening issues to that part...

But, yep, I understand what is happening here... return a :floating from an :integer is not a bug or a bad thing, for instance.

drkameleon commented 1 year ago

Funny enough... I tried reproducing the supposed "issue":

x: 2
print ["x before:" x]
'x + 2.1
print ["x after:" x]

f: function [z][
    z + 4.1
]

k: 3
print ["k before:" k]
f 'k
print ["k after:" k]

The "key" here is that we are in-place modifying an Integer that - given that we add a Floating to it - it becomes a Floating, so its type is changing.

The "problem" is that this gives:

x before: 2 
x after: 4.1 
k before: 3 
k after: 7.1

...which is exactly what it should be! 😲 (I was sure the 2nd example - f 'k - would trigger it, but it doesn't!)

Perhaps there is no issue whatsoever?! Geez! lol

drkameleon commented 1 year ago

Hmmm... 🤔 I'll try it with a different function, one using SetInPlace in its implementation, but it shouldn't make much of a difference...

drkameleon commented 1 year ago

Ultra-weird... I'm losing my mind now:

y: `h`
print ["y before:" y]
'y ++ "ello"
print ["y after:" y]

g: function [w][
    w ++ "ello"
]

m: `h`
print ["m before:" m]
g 'm
print ["m after:" m]

Result:

y before: h 
y after: hello 
m before: h 
m after: hello

Looks like it's fine?! Perhaps this "issue" is a leftover from when the issue persisted? Perhaps I'm not triggering it?! Let's see...

stale[bot] commented 12 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 7 months ago

Closing issue as stale.