Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variable does not get replaced in assignment expression #830

Closed
tiymat opened this issue Jul 10, 2024 · 10 comments · Fixed by #849
Closed

Variable does not get replaced in assignment expression #830

tiymat opened this issue Jul 10, 2024 · 10 comments · Fixed by #849
Labels
design question This is where the murex devs can raise a feature idea or discuss a problem with the community

Comments

@tiymat
Copy link
Contributor

tiymat commented Jul 10, 2024

Describe the bug:
In an assignment expression where the term on the left side of the = is a variable (e.g. $foo), the expression on the right of the = gets assigned to the variable's name, not the variable's value.

Expected behaviour:
Either the value of the variable should get assigned the value on the right of the =, or the documentation should be updated to clarify that variables cannot be assigned in this way.

Screenshots:
image

Platform (please complete the following information):

  • OS, output from uname -a if supported: 6.9.7-arch1-1
  • Terminal Emulator: kitty 0.35.2
  • Murex version, output from version --no-app-name: 6.1.8300

Additional context
From the syntax highlighting in the screenshot where the equals sign is green, I thought potentially it was just failing because it was parsing the = as part of the $foo variable. But adding a space on either side of = led to the same issue.

@tiymat tiymat added the bug Unexpected behavior label Jul 10, 2024
@lmorg
Copy link
Owner

lmorg commented Jul 10, 2024

This is intended behaviour. But I don't really know if I like it.

There was a discussion about it in: #705 (comment)

I'd appreciate you're thoughts too.

Ostensibly it comes down to whether the inclusion of $ acts like a pointer.

Reasons to keep as is:

  • Having $ vs no sigil does make the syntax easier for new people
  • there is already scripts written where the sigil is included in assignments, so changing it back would cause hard-to-catch bugs
  • documentation is already written where the sigil is included in assignments. eg https://murex.rocks/tour.html#global-variables

Reasons to change:

  • making the sigil optional basically requires a large amount of hacks. So there are going to be lots of edge case parsing bugs where $foo will be translated to "value of $foo" even though the docs suggest its "the variable named $foo"
  • consistency with other shells, eg Bash
  • allows greater freedom in code (eg metaprogramming)

Personally, I regret making this change. I think it adds more complications and bugs to Murex's source code than the easy of use benefits it creates for users. But changing it could silently break lots of scripts. So I really don't know the best way to approach this

@lmorg lmorg added design question This is where the murex devs can raise a feature idea or discuss a problem with the community and removed bug Unexpected behavior labels Jul 10, 2024
@tiymat
Copy link
Contributor Author

tiymat commented Jul 11, 2024

Although I might quibble with your point about having $ making it easier to pick up for new learners, I take your points about how changes to this behavior would break both people's scripts and understanding of the language.

In a world in which making the change had 0 cost in terms of development effort and existing users having to adapt their scripts, I would support changing it. I think the metaprogramming potential alone is compelling enough reason.

In this world, however, I guess it comes down to cost vs. benefit. I don't see this as a huge issue, just something behaving differently than I'd expect, and I suspect changing it might be more trouble than it's worth.

@lmorg
Copy link
Owner

lmorg commented Jul 11, 2024

I've been thinking about how i might revert this change, and I think it might be possible to do it relatively safely.

  1. update the docs immediately
  2. write a tool that checked for the existence of $. Basically it shouldn't need to be more sophisticated than a few regexp expressions
  3. in the new release greater (the one that appears when you start the shell for the first time with a new version), display the deprecation warning and how to use the static analysis tool

This would need to be a something pegged for v8.0 though I suspect. So would take a while for it to hit master.


I've also considered the option of having this behaviour configurable. Like with strict-vars, a proc option, maybe named ignore-sigil or meta-programming. But basically haven't it disabled at first and then come v8.0 it can be enabled by default.

I might go with that too actually. It would then act a little like an unsafe block in that it makes it very clear to readers and the parser what the intention of the code is, and thus easier to handle deprecation warnings.

@tiymat
Copy link
Contributor Author

tiymat commented Jul 12, 2024

So the idea would be in v7 deprecation warnings would be printed, giving time for people to fix things up before v8? Sounds reasonable.

Honestly, though, I'm a little hesitant about making the behavior configurable in the way you're describing. I don't love the idea of adding the cognitive load of needing to keep in mind the config just to understand what an expression as simple/fundamental as $foo="bar" will do. Just my 2 cents though.

@lmorg
Copy link
Owner

lmorg commented Jul 17, 2024

@tiymat I can't deprecate this. I completely forgot that unicode variables depends on this feature: https://dev.murex.rocks/parser/scalar.html#unicode-variable-names

@lmorg
Copy link
Owner

lmorg commented Jul 18, 2024

I think I might have figured out a way to retain current compatibility while also enabling support for meta programming:

» $foo = "bar"
» $VALUE_OF.foo = "baz"
» $bar
baz

(this code hasn't been written yet. It's just a proposal)

So basically $VALUE_OF would get pre-parsed and replaced with the value of whatever the value of the variable that follows it.


This would require two changes to the way how variables currently work:

  1. variables returned as objects will need to be a little smarter and have object code separated from meta programming values. However this would also bring gains for objects like path and paths to make them a little easier to use too.
  2. commands will need a two stage variable parser. However this would also solve a lot of the edge case bugs where $SCALAR behaves unexpectedly with some commands due to it sometimes parsing the variable as a scalar and sometimes as a value.

@lmorg
Copy link
Owner

lmorg commented Aug 4, 2024

Might have an even easier solution to this problem: parenthesis

image

@lmorg
Copy link
Owner

lmorg commented Aug 4, 2024

The potential risk here is that:

  • $(foo) is the variable named "foo"
  • ($foo) is the value of the variable "$foo"

So transposing the opening bracket would cause unexpected behaviour.

....however....

this design would also all in line with another change I want to make, and that's supporting variables inside dot notation. eg

$foo.($bar).baz

Which would basically be:
Get the element /xxx/baz from $foo where xxx is the value from $bar

@lmorg
Copy link
Owner

lmorg commented Aug 4, 2024

Pushed some prototype code to develop for the assignment idea. Lots of tests breaking as a result though, so there might be some instabilities created by this commit that need to be identified should we decide to proceed with this syntax

@tiymat
Copy link
Contributor Author

tiymat commented Aug 6, 2024

The parenthesis syntax is a neat idea!

@lmorg lmorg linked a pull request Aug 17, 2024 that will close this issue
@lmorg lmorg mentioned this issue Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design question This is where the murex devs can raise a feature idea or discuss a problem with the community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants