Modify

Opened 11 years ago

Closed 11 years ago

#2478 closed Bug (Fixed)

Assign and Eval do not restrict variable names

Reported by: Tasiro Owned by: Jon
Milestone: 3.3.11.1 Component: AutoIt
Version: 3.3.9.21 Severity: None
Keywords: assign, eval, name, restriction Cc:

Description

Local $name = "some space, and a comma"
Assign ($name, 42)
Eval ($name) ; => 42

Is that behavior intended? Should such inaccessable names be allowed?

Attachments (0)

Change History (13)

comment:1 follow-up: Changed 11 years ago by MHz

Interesting. I could consider it as a desirable feature.
For instance.

Local $name = "Question, what is 3 + 3?"
Assign($name, 6)
MsgBox(0, '$name', $name & ' ' & @CRLF & Eval($name)  & @CRLF)

You intend to access the string representation of the variable with Eval so I see only benefit. What the name in the variable could be regarded as little concern. You have string functions to check if what is in $name is valid or not by your own criteria. Same as you test for what is put in an edit control of a gui.

comment:2 Changed 11 years ago by anonymous

Note my testing was done in 3.3.8.1 so this feature has been out in the wild for quite some time.

comment:3 in reply to: ↑ 1 Changed 11 years ago by Tasiro

Replying to MHz:

Interesting. I could consider it as a desirable feature.
For instance.

Local $name = "Question, what is 3 + 3?"
Assign($name, 6)
MsgBox(0, '$name', $name & ' ' & @CRLF & Eval($name)  & @CRLF)

You intend to access the string representation of the variable with Eval so I see only benefit. What the name in the variable could be regarded as little concern. You have string functions to check if what is in $name is valid or not by your own criteria. Same as you test for what is put in an edit control of a gui.

I am afraid I must disagree. There are several disadvantages:

  1. It makes it harder to debug.
  2. It may polute the environment (and makes any good optimization virtually impossible).
  3. The environment is not a map and should not be used as a map.
  4. You cannot access the variable the normal way (no $"Question..." or $$name allowed).

All that glitters is not gold.

I would even deprecate Assign, Eval, IsDeclared, Call and Execute (Call and Execute might have side effects). That kind of reflection should be unneccessary.

comment:4 follow-ups: Changed 11 years ago by MHz

  1. It is a string. You can test it before using assign so I see this as not an issue.
  2. As above.
  3. So you say but that does not mean that it should not be.
  4. This is why Eval exists. Since when has $"Question..." been allowed? or even $$name? or how about $"var". Perhaps you are thinking of setting the option on for ExpandVarStrings which works fine on "name = $some space, and a comma$" if you wanted to.
  5. Additional point, why submit bugs reports on functions that you do not want to use?

In relation to your mention of gold, gold out of the ground is not good unless it is processed into pure gold. So processing is usually needed.

You can test the string before assigning it for characters you do not want. If you assign a string to become a variable, then you can test it with IsDeclared to test if it exists and if it does, then to what scope it is and you can use Eval to use it. The latter is mentioned in the help file as what you should do on the pages of Assign, IsDeclared and Eval.

Below is code testing the string for being valid for your interest. Added additional _IsDeclared UDF to show Assign, IsDeclared and Eval working together. You can comment the IsAlphaNumUnderScore function to see that it was not allowing the string. You could block assigning the string, convert the string to suit, exit the script or what ever you choose to do. I fail to see how changing the behavior of Assign does any good as you are going to have to check for error and deal with that with more code to correct it. And it is a script breaking change for no good reason that I see at this time.

Tested on 3.3.8.1 and 3.3.9.21. 1 function call tests if the string is just letters, numbers and underscores.

$name = 'some space, and a comma'

If _IsAlphaNumUnderScore($name) Then
	If Assign($name, 46) Then
		MsgBox(0, 'Success', _
			$name & ' = ' & Eval($name) & @CRLF & _
			'Scope = ' & _IsDeclared($name) & @CRLF _
		)
	EndIf
EndIf

Exit

Func _IsAlphaNumUnderScore($string)
	; success if the string is all letters, numbers or underscores
	If StringRegExp($string, '[^a-zA-Z0-9_]') Then Return SetError(1, 0, 0)
	Return 1
EndFunc

Func _IsDeclared($string)
	; IsDeclared but return strings
	Assign('Local: Scope', IsDeclared($string), 1)
	If Eval('Local: Scope') Then
		Switch Eval('Local: Scope')
			Case -1
				Return 'Local'
			Case 1
				Return 'Global'
		EndSwitch
	EndIf
	Return ''
EndFunc

Here is a tip, variables created with Assign, i.e. "var!" will not destroy a variable named $var. You cannot create a variable named $var! so it is safe to assign "var!".

So I do not see an issue, like AutoIt crashing, negative side effects or other. So as per usual with AutoIt, some programming is needed to get the result that you want.

comment:5 Changed 11 years ago by MHz

Perhaps a request for additional flags?

0 = (default) Create variable if required
1 = Force creation in local scope
2 = Force creation in global scope
4 = Fail if variable does not already exist
8 = Fail if variable does already exist (new)
16 = Fail if not being a string with letters, numbers and underscores (new)

comment:6 Changed 11 years ago by Jon

Bug as far as I'm concerned.

comment:7 in reply to: ↑ 4 Changed 11 years ago by Tasiro

  1. Bad Idea. "You can't access my variables? Have you tried Eval ("variable name") already?"
    Also, Execute may fail. What will be @extended?
  2. Execute is the (second) slowest way possible.
  3. Use ObjCreate ("Scripting.Dictionary") for maps.
  4. Again, it is a bad idea to use Assign, Eval and Execute. See https://en.wikipedia.org/wiki/Eval.
  5. I cannot deprecate functions of the standard library of AutoIt myself, can I?
    Should I not report bugs I find?
  6. Where is Eval actually neccessary?

I do not think a new flag would be a good idea.

Documentation - Language Reference - Variables:

Each variable has a name (again, similar to a mailbox) and must start with the $ character and may only contain letters, numbers and the underscore _ character.


Anyway, here is my flag proposal:
16 = (not default) Do not fail if varname not being a variable-name (link to specification)

comment:8 in reply to: ↑ 4 Changed 11 years ago by Tasiro

I fail to see how changing the behavior of Assign does any good as you are going to have to check for error and deal with that with more code to correct it.

Then I will have to? Only IsDeclared needs to be changed.

And it is a script breaking change for no good reason that I see at this time.

You cannot expect undefined (or unspecified) behavior to be portable (you seem to be in need of a specification).
Which script would it break?

Here is a tip, variables created with Assign, i.e. "var!" will not destroy a variable named $var. You cannot create a variable named $var! so it is safe to assign "var!".

I may write Assign ("var!", $value), as you do.
Why do you need a variable named "var!"?

So I do not see an issue, like AutoIt crashing, negative side effects or other.

Performance. Consistence. Misuse.

comment:9 Changed 11 years ago by Jpm

Hold on
Jon acknowledge it is a BUG , I am sure you will be OK with the fix when he will do it

comment:10 Changed 11 years ago by BrewManNH

One benefit that I've seen, in Assign and IsDeclared taking any string as a valid variable, is some of the code people have used to replace the _ArrayUnique function with something a lot faster. While it's not standard practice to use Assign like that, it has been something people have been posting for several years and in multiple posts. Just something to think about.

comment:11 Changed 11 years ago by czardas

Ah, you're going to change it.

Although I didn't know about this previously, I think it would be pretty cool if it remained as an undocumented feature. If there are potential issues with behaviour then it should be changed, otherwise I don't see any reason or benefit to change anything. The fact this is even possible seems to open up new possibilities. I made such a function as BrewManNH spoke of, but all variable names use word characters after conversion to hex - allowing case sensitive variable names.

comment:12 Changed 11 years ago by anonymous

Stripping, restricting or rejecting whitespace/punctuation from the variable name parameters doesn't seem too harsh. Though I don't see the necessity in expending effort to dismantle the entire set of functions.

Is this like a chicken farmer showing his banker how he raises chickens in a sanitary environment, uses only organic feed, how he is producing a bunch of happy, healthy, free-range chickens, and near the end of the tour, they hear calls of "Whoop! Whoop!" and "Yee Haw!" and glance up to see the farm across the road where, in a cloud of dust and feathers, that entire family are out f*cking their chickens.
"I must put a stop to this!", he says?

comment:13 Changed 11 years ago by Jon

  • Milestone set to 3.3.11.1
  • Owner set to Jon
  • Resolution set to Fixed
  • Status changed from new to closed

Fixed by revision [9505] in version: 3.3.11.1

Guidelines for posting comments:

  • You cannot re-open a ticket but you may still leave a comment if you have additional information to add.
  • In-depth discussions should take place on the forum.

For more information see the full version of the ticket guidelines here.

Add Comment

Modify Ticket

Action
as closed The owner will remain Jon.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.