The default value for untyped undefined parameters

[Anatoliy057]

Example:

proc _test(@var1, string @var2) {
    console(@var1 == '')
    console(is_null(@var2))
}
_test()

Out (Error omitted):

true
true

[Anatoliy057]

I think the problem is here

CommandHelper/src/main/java/com/laytonsmith/core/functions/DataHandling.java

Line 1263 in 9e938ad

c = new CString("", t);

If replaced with CNull.UNDEFINED, the test will fail:

CommandHelper/src/test/java/com/laytonsmith/core/MethodScriptCompilerTest.java

Line 339 in 9e938ad

public void testExecute16() throws Exception {

[LadyCailin]

How does it fail, does it msg "1 null null" etc?

[Pieter12345]

I believe the overall issue here is that these default values are used in the first place. If you want to make a proc parameter optional, a default should be provided as proc _p(@mandatory, @optional = null, @optional2 = '') {...}. At the moment, there is nothing that checks this and users might check for null or '' to see if their typed and untyped arguments respectively are provided or not. That means that changing the default of one to the other will break scripts, and worst of all, silently passes these checks inserted by the user. Therefore I think that this issue should not be changed until procedure signatures can be checked. A proper place to do this would be in a static analysis, generating compile-time errors when these signatures do not match (and one of the defaults from this PR would be used).

[Anatoliy057]

Fail due to mismatch, actually messages are:

• "null null null"
• "1 null null"
• "1 2 null"
• "1 2 3"
• "1 2 3"

[LadyCailin]

Hmm. I think I agree with Pieter here. No matter how we fix this, we will violate some user's assumptions about what the default value should be. Either we pick default to empty string for untyped variables and explicitly typed to string values, and null for all other types, or we pick default to empty string for untyped values, and default to null for all typed variables. In either case, someone's code behavior might change, and that's not acceptable without some deprecation period, or at least a pre-runtime check.

However, it might be that we can't implement pieter's suggestion either, until we get varargs implemented, because there is definitely perfectly valid use cases for sending more values than specified in the signature. But I guess in this case, you're sending less, so perhaps the short term measure in the static analysis would be that you can always send more arguments that specified, but never less, (unless they are explicitely optional via string @b = '' or whatever).

Thoughts?

[Anatoliy057]

I agree that this can be left, because without change it is still easy to understand that there are not enough arguments. I just wanted to write what I found on this issue.

It will maybe possible to fix it when there is a serious update, which will entail the need to modify old scripts anyway.

[LadyCailin]

As long as we put a compile time error, I'm much more comfortable making changes like this incrementally, especially when it points to some probably actual weirdness in user code.

But anyways, thanks for the report!