CFLint icon indicating copy to clipboard operation
CFLint copied to clipboard

New rule to detect when array are passed in CF

Open justinmclean opened this issue 7 years ago • 7 comments

Arrays in CF unlike other languages are passed by value rather than reference. This can cause all sorts of issues for people unaware of this.

justinmclean avatar Aug 11 '17 02:08 justinmclean

I'm not sure how I feel about a purely informational rule. There's nothing in the code you can reasonably change to "fix" it. CF2016 at least has the application setting passArraybyReference, but I don't think CFLint would take that into account. That's also making an assumption about the programmer. Passing by reference seems as though it could just as likely cause issues if the programmer is unaware of how it's set.

EDIT: By the way, Lucee just always passes arrays by reference.

KamasamaK avatar Aug 11 '17 06:08 KamasamaK

Not pass an array? I have seen people bitten by this but if no one else think's it's important OK to close and not implement.

justinmclean avatar Aug 11 '17 06:08 justinmclean

@justinmclean I'm not sure what's a reasonable alternative to an array. If they're simple values then I suppose it could be a list instead, but that's passed by value as well and I've always preferred to use an actual data structure anyway. A wrapper component seems like it would be overkill in most cases, but something to consider I guess.

KamasamaK avatar Aug 11 '17 07:08 KamasamaK

I'm on the fence. I think the intention of the rule would be good.

There are ways to "code around" it but converting the array to something else, if you really wanted to.

People are not forced to run the rule either? If the do choose to include it but want to ignore individual instances, they could do that too.

TheRealAgentK avatar Aug 12 '17 07:08 TheRealAgentK

What do you think of only flagging cases where the array is a parameter, AND

  • It has an assignment that potentially changes the context
  • OR, it passes it on to another function. ?

ryaneberly avatar Aug 12 '17 14:08 ryaneberly

@ryaneberly If we are going to have a rule like this, then I guess I would be OK with the first option. There's nothing wrong with passing an array, but if you modify it then maybe it could present something indicating that the original array might not be modified depending on your application setting (if your engine supports it).

KamasamaK avatar Aug 12 '17 17:08 KamasamaK

To be clear, the reasons I'm not a fan of this rule as presented are:

  1. I think the tool should only be concerned with reporting issues and best practices, and I don't think this is either.

  2. I think it's a slippery slope to start creating rules that are just "CFML behaves differently from other languages in this respect" or "this thing might not behave how you might expect" (unless it's actually buggy like isDate). That's something that a developer would only need to be told once and not reported for each occurrence. In this case in particular, assuming the developer was not aware before, that difference should bear out in development and testing when it doesn't work as expected. I suppose the opposite case where you expect it to be passed by value and it's instead passed by reference could cause an issue that is not immediately apparent, but again this is just something the developer should be aware of.

  3. The "fix" would be to not use an array, which is a perfectly fine data structure.

Regarding the comment that no one is forced to use the rule, this could be said about any rule and is not a good argument itself. But more to that point, perhaps it could be useful to have a mechanism that could have a rule be outside of the "core" rules that are included by default and be made to only be explicitly "included". I think there's a certain responsibility associated with the core rules, and I feel the vast majority (all?) of developers would simply exclude this rule anyway.

I think the suggestion by @ryaneberly steers it toward more reasonable as a rule, but most of my criticism still applies.

KamasamaK avatar Aug 12 '17 18:08 KamasamaK