richpl / PyBasic

Simple interactive BASIC interpreter written in Python
GNU General Public License v3.0
170 stars 46 forks source link

Incorrect string array initialization #61

Closed RetiredWizard closed 2 years ago

RetiredWizard commented 2 years ago

Hi Rich,

I hope you're enjoying the Holidays :-)

I've run into a memory leak which could be in any of CircuitPython, PyDOS or PyBasic. While poking around trying to figure out how to narrow down the possibilities I noticed an issue with PyBasic arrays.

The BASICArray class will initialize array values to zero, however it doesn't have variable type information so string arrays get initialized to zero (rather than presumably a NULL string).

This isn't really a show stopper since an easy work around is to explicitly initialize any string arrays after dimensioning them.

It's been a while since I've worked on PyBasic and I didn't want to throw a solution PR at this without giving it my full attention. My quick thoughts were that a type flag (ie String/Numeric) could be passed into the BASICArray init method so the proper initialization can be done.

This code snippet demonstrates the issue:

      10 DIM A$(10)
      20 T$ = A$(1)
      30 PRINT T$
      > run
      Syntax error: Attempt to assign non string to string variable in line 20

And here it is with the workaround:

      10 DIM A$(10)
      15 FOR I = 1 TO 10
      16 A$(I) = "":NEXT I
      20 T$ = A$(1)
      30 PRINT T$
      > run
richpl commented 2 years ago

Thanks @RetiredWizard, good spot. I'll take a look and will get back to you.

Did you really post this issue on Christmas Day? I'll admit to once pushing an update to GitHub at nearly midnight on New Year's Eve, but I've felt bad about it ever since!

richpl commented 2 years ago

Actually I've just take a look @RetiredWizard and I think you are right. Depending upon whether the name of the variable in the __dimstmt method has a dollar suffix, the type of the array can be determined and passed to the BASICArray initialiser as an extra parameter as you suggest. Do you want me to make the change or are you happy to have a crack?

RetiredWizard commented 2 years ago

Yep Christmas day is a day for me time (well and some family time :) ) and playing with PyDOS and PyBasic definitely falls in the me time activities :D.

I'm pretty focused on the Raspberry Pi Zero 2w build of CircuitPython right now so I probably wouldn't pick this one up for a while. If you think you've got a handle on it and want to knock it off, go ahead. If you'd rather not deal with it then I'll pick it up eventually.

richpl commented 2 years ago

Roger that, I'll work on the fix over the next few days

richpl commented 2 years ago

Apologies @RetiredWizard, it's taken me an age to get around to this, but just committed a fix for this. Let me know if you think I've broken anything. I tested with regression.bas and eliza.bas.