Maptool: Mixing case-insensitive parsing with case-sensitive evaluation produces numerous unexpected results

[selquest]

Describe the bug
After looking into the recent getname() -> setName() issue (#1950), it's clear that MapTool is very inconsistent in its approach to case sensitivity for function calls:

1. Several examples can be found of functions incorrectly behaving as completely different functions:
   
   
   
   • getplayername() behaves as getAllPlayerNames()
   
   
   • addallpcstoinitiative() behaves as addAllNPCsToInitiative()
   
   
   • removeallpcsfrominitiative() behaves as removeAllNPCsFromInitiative()
   
   
   • setcurrentinitiative() behaves as getInitiativeToken()
   
   
   • presumably more than a few others - (Note: for examples here I'm using all lower case in the function calls, but any deviation from the "correct" capitalization has these results)
   
   
2. Other functions behave normally no matter what case is used:
   
   
   
   • base64.ENcode() is accepted for base64.encode()
   
   
3. Some functions produce ParserExceptions (unknownFunction) when called with improper case:
   
   
   
   • broadCast() -> "Unknown function: broadCast" (as opposed to broadcast())
   
   

Expected behavior

1. Incorrectly casing a function call should definitely not result in calling a completely different (and sometimes exactly opposite) function.
2. Some consistency in expectations for function calls to be case-sensitive or not.
   
   
   
   • From familiarity with other language conventions, I would expect function calls to be case-sensitive. If getName() is a defined function, I would not expect to be able to call it via getname().
   
   

MapTool Info

• Version 1.7

Additional context

Category 1:
The major issues from category 1 above are mainly results of Function implementations using logic like:

if(functionName.equals("getName"){
  // execute getName() function
} else {
  // assume we must be in setName() without checking
}

This is definitely not desired, especially while we continue to use a CaseInsensitive map in the parser and are inconsistent in applying case sensitive comparisons in the evaluators.

Case sensitivity as a general principle:
The consensus from discussions on discord seemed to prefer the enforcing of strict case sensitivity for function invocations (as is typical for programming languages), but there are concerns about backwards compatibility with existing frameworks. I suspect that cases like category 2 above are rather in the minority, but breaking even just that single example found so far would have the potential to negatively impact existing macros. Getting a more complete picture of the state of case sensitivity in function implementation will help to judge the relative risk/reward of any change made on this.

Action Plan

1. I'll start by trying to get a better inventory of macro functions and their current state of case-sensitivity. From there we can see just how big an impact any change here will have.
2. Eliminate the improper fall-through execution examples that allow for function calls to be accidentally interpreted as completely other function calls.
   
   
   
   • throw "Unknown function" ParserExceptions in the hopefully-soon-to-be-unreachable cases of unmatched functionNames
   
   
3. Reach a consensus on standardizing for case-sensitive or case-insensitive function calls, based on inventory and determined impact.
4. Apply chosen standard: (Note: because of the potential for breaking changes, it may be desirable to delay implementation until a new major release)
   
   
   
   • Add chosen standard to the Code Style and Guidelines or other developer documentation
   
   
   • If standardizing to case-insensitive:
     
     
     
     
     
     • replace functionName.equals() comparisons with .equalsIgnoreCase()
     
     
     
   
   
   • If standardizing to case-sensitive:
     
     
     
     
     
     • switch the parser to use a case-sensitive function map
     
     
     
     • replace any existing case-insensitive comparisons with strict casing
     
     
     
     • update the macro editor syntax parser to require correct case when matching built-in functions
     
     
     
   
   

[Merudo]

I thought the agreed-upon solution was to make all function names non-case sensitive (#132):

I think the issue here is case sensitivity.

The Parser is not case sensitive. So if your macro calls "getname" in all lowercase, the parser recognises that as being covered by the TokenNameFunction class and sends it there.

However the TokenNameFunction class makes case sensitive comparisons, and only "getName" is treated as that function. Whereas "getname" will be treated the same as "setName".

All functions should match using "equalsIgnoreCase" rather than "equals"

_Originally posted by @Jaggeroth in https://github.com/RPTools/maptool/issues/132#issuecomment-367857761_

[selquest]

I'll review and pretty up the results tomorrow, but my first pass inventory shows that of the 571 function names registered with the parser on a startup of the current development branch:

• 305 (53.4%) are some form of case-sensitive
  
  
  
  • 177 functions that throw ParserExceptions when case doesn't match
  
  
  • 95 functions that have some kind of different return value when case doesn't match (specific return value varies)
  
  
  • 33 functions where incorrect case leads to the wrong function being called
  
  
• 247 (43.3%) are some form of case-insensitive
  
  
  
  • 132 functions where functionName is compared ignoring case
  
  
  • 91 functions where a Function implementation only provides one operation, so the case-insensitivity is applied by the parser
  
  
  • 24 functions which are sort of accidentally case-insensitive, by virtue of being the default execution path in a logic structure like the example above
  
  
• 15 (2.6%) are operators like + and <, and case-sensitivity has no meaning. (most of these have text aliases like add(), and those are counted in the stats above)
• 4 (0.7%) are undetermined - some logic paths that collapsed multiple aliases into default branching, and I wasn't able to easily confirm whether alternate casing produced the correct result

So assuming these numbers hold up after a second pass, it looks like the major problem of inconsistent handling of case is not far off from 50/50. That's definitely more than I was expecting to be ignoring case after my initial exploration, but it makes sense in retrospect as the overwhelming majority of functions defined in the parser project use a Function implementation providing a single operation - though often with multiple aliases like add()/sum()/concat().

Switching to fully case-sensitive function names would definitely provide a lot of opportunities for conflicts with existing frameworks. While I think I'd still prefer to switch, it should probably not happen outside of a major version release that is already expected to break backward-compatibility.

[Phergus]

• 95 functions that have some kind of different return value when case doesn't match (specific return value varies)
• 33 functions where incorrect case leads to the wrong function being called

These are problems that need to be corrected. If there are frameworks using the functions with an incorrect case they aren't working correctly already. With the new changes that produce a stack trace for macro errors, finding and fixing the issues in a framework won't be nearly so difficult as before.

[selquest]

More complete findings now via Published Google Sheet.

I thought the agreed-upon solution was to make all function names non-case sensitive (#132):

@Merudo Yeah, I'd seen those comments when I first started looking into this, and was wondering whether it still held - whatever the preference, it clearly hasn't been widely standardized. Some quick discussion on Discord seemed to indicate an overwhelming preference in the other direction - but with concerns for backwards-compatibility.

These are problems that need to be corrected. If there are frameworks using the functions with an incorrect case they aren't working correctly already.

@Phergus Agreed. I figure fixing these up is the next step - the Wrong Function cases in particular. I also think we should clean up the default execution instances to better protect against unexpected execution flow in the future, but the nature of those changes would depend on our ultimate objective.

The main question remains on the overall goal: if we want function name matching to ignore case, then we should also take steps to implement the flexible comparisons on the remaining functions (perhaps less urgently on the functions that are throwing ParserExceptions right now). If we want function name matching to eventually enforce specific case, then we should standardize around the ParserException pattern for those functions that are currently case-sensitive, and more carefully plan a roll out of changes to the functions that currently ignore case.

[selquest]

For right now, assuming that we aren't quite sure on the ultimate goal for case-sensitivity, but don't want to break functions that currently ignore casing, I'm thinking my Step 2 is:

• Eliminate the Wrong Function cases by throwing ParserExceptions. As Phergus said, those functions are not currently working even if the macros that use them have somehow managed to wrap those outcomes into their results. A ParserException would indicate where an invalid function is being called, and be the best help to resolving it.
  
  
  
  • When eliminating the default execution pattern, maintain the case-insensitivity I call "Accidental" by using .equalsIgnoreCase() for only the function that was formerly the default.
  
  
• Switch the "special Return Value" cases over to produce ParserExceptions. Same reasoning as above.

[cwisniew]

For right now, assuming that we aren't quite sure on the ultimate goal for case-sensitivity, but don't want to break functions that currently ignore casing, I'm thinking my Step 2 is:

• Eliminate the Wrong Function cases by throwing ParserExceptions. As Phergus said, those functions are not currently working even if the macros that use them have somehow managed to wrap those outcomes into their results. A ParserException would indicate where an invalid function is being called, and be the best help to resolving it.

  • When eliminating the default execution pattern, maintain the case-insensitivity I call "Accidental" by using .equalsIgnoreCase() for only the function that was formerly the default.
• Switch the "special Return Value" cases over to produce ParserExceptions. Same reasoning as above.

I wish it was all case sensitive but something about a horse and a gate. Given that next to none of the existing code will be used for MTScript2 and MTScript 1 will go into "legacy mode" with few updates at that point, I think I would lean towards breaking as little as possible. So I think this approach is best compromise.

[selquest]

Those 2 PRs handle the biggest issue: function calls being executed as completely different functions.

Next up is the "Return Value" category. There are more of these, so they'll take a bit longer. Doing this as multiple PRs to try and avoid merge issues, since I'm obviously touching a lot of files here.

[Phergus]

Dicelib bumped to 1.6.4 with latest fixes for this issue.

• RPTools/dicelib#52
• RPTools/dicelib#53

[Phergus]

@selquest should this be considered complete?

[selquest]

@Phergus I think given the state of existing functions and ongoing plans for the future, this is as much as should be done right now, yes. We're still left in a state where we're inconsistent on case sensitivity for function calls, but as commented above - horse, barn, gate. With the parser 2.0 I hope the opportunity is taken to take a stance on casing from the beginning.

In the meantime, these PRs should have fixed the most glaring issues, and that was definitely the priority.

[Phergus]

Very good. That was my take but wanted to make sure you didn't have anything pending.