nmodl icon indicating copy to clipboard operation
nmodl copied to clipboard

Fix bug with state named 'is'.

Open 1uc opened this issue 1 year ago • 16 comments

Sympy doesn't tolerate variable names called is.

1uc avatar May 14 '24 16:05 1uc

Fixes #1258.

1uc avatar May 14 '24 16:05 1uc

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.32%. Comparing base (2d9e7c4) to head (0b48310).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1263   +/-   ##
=======================================
  Coverage   86.32%   86.32%           
=======================================
  Files         175      175           
  Lines       13219    13219           
=======================================
  Hits        11411    11411           
  Misses       1808     1808           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar May 14 '24 18:05 codecov-commenter

Will still break if the following keywords are declared as NMODL variables:

'nonlocal', 'except', 'else', 'from', 'not', 'with', 'for', 'or', 'try', 'and', 'class', 'None', 'True', 'async', 'continue', 'break', 'False', 'if', 'return', 'assert', 'await', 'global'

Other than that, a much better solution that what I had in mind.

JCGoran avatar May 16 '24 09:05 JCGoran

I understand that it's rather partial solution. Some are tricky: should if be a legal variable name? I don't know and I want to punt. Same for global. I'd be quite happy if it continues to crash for all of those.

The problem is we only have strings. We don't know if a particular substring is a variable or if it's a keyword. We know if a particular string is used as a variable name; but we don't know if a particular occurrence of the string is a variable or a keyword.

1uc avatar May 16 '24 09:05 1uc

Re: legality of variable naming, shouldn't NOCMODL be the reference for what is valid and what isn't (however strange the rules may be)? In any case, the variables which NOCMODL doesn't seem to like (out of the ones I listed above) are apparently else not for or and if return assert, while the other ones compile fine, so maybe we can just add those in the list of variables to rename and call it a day for now.

JCGoran avatar May 16 '24 10:05 JCGoran

Makes sense. The proper place to fix all of this is when NMODL create the string it sends to SymPy. That string should not contain any troublesome identifiers, i.e. all variable should probably be mangled first; and demangled after SymPy. There we'd have a better understanding of each substring.

1uc avatar May 16 '24 11:05 1uc

I'm surprised class and try would work. Edit: Liberal use of #define makes it work.

1uc avatar May 16 '24 11:05 1uc

I'm surprised class and try would work.

NOCMODL seems to define those, which is apparently legal (though it raises a warning on clang): https://godbolt.org/z/sa9qddYTY

JCGoran avatar May 16 '24 11:05 JCGoran

It feels very strange to allow C++ keywords as a variable names. It feels even more off to "fix" it by patching the string sent to SymPy, meaning the C++ keywords are returned from SymPy again. There's a better way of doing this by adding an early pass which renames any variable names that are C++ or Python keywords to something safe. Then we don't need to do any trickery with the string to be sent to SymPy.

I get your point of nocmodl being the reference. Since it's all not very clean, I've reduced the number of keywords we sanitize in this unsatisfactory way to a small number of what I believe a plausible variable names. I'd be open to reducing what works even futher to just is and maybe in, to make the bug Koen is seeing go away.

1uc avatar May 16 '24 15:05 1uc

Since keeping a list of both C++ and Python keywords is a bit cumbersome, we could add prefix/suffix to all variables internally or something, but we can postpone the details of that discussion for later. For now, as long as this PR fixes the issue, I'm fine with it as a temporary workaround.

JCGoran avatar May 16 '24 16:05 JCGoran