Fix bug with state named 'is'.
Sympy doesn't tolerate variable names called is.
Fixes #1258.
Logfiles from GitLab pipeline #211054 (:no_entry:) have been uploaded here!
Status and direct links:
Logfiles from GitLab pipeline #211065 (:white_check_mark:) have been uploaded here!
Status and direct links:
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.
Logfiles from GitLab pipeline #211096 (:white_check_mark:) have been uploaded here!
Status and direct links:
Logfiles from GitLab pipeline #211224 (:white_check_mark:) have been uploaded here!
Status and direct links:
Logfiles from GitLab pipeline #211252 (:white_check_mark:) have been uploaded here!
Status and direct links:
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.
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.
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.
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.
I'm surprised class and try would work. Edit: Liberal use of #define makes it work.
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
Logfiles from GitLab pipeline #211535 (:no_entry:) have been uploaded here!
Status and direct links:
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.
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.
Logfiles from GitLab pipeline #211937 (:no_entry:) have been uploaded here!
Status and direct links:
Logfiles from GitLab pipeline #212210 (:white_check_mark:) have been uploaded here!
Status and direct links: