CodeChecker, checking an application using ColdBox
First things first-- almost everything the codechecker reports to you is a suggestion. The code checker has hundreds of rules, many based on preference, suggestion, or just abundance of caution. If the code checker flags something, it doesn’t mean it’s wrong, it simply means you should look at it and see if it’s a problem or not.
reporting many ‘unscoped CFC variables’ in the ColdBox framework.
There’s nothing wrong with “unscoped” variables. Every variable is in a scope, it’s just a matter of whether the scope has been explicitly declared. The default scope in a CFC is the “variables” scope. So
foo = “bar”;
and
variables.foo = “bar”;
are the same effective code.
We require our hosted apps to explicitly scope all ColdFusion variables
That’s fine if it’s your coding standard, but it’s a little draconian in my view. I’m not aware of any other shops with such a rule and I think it really undoes a lot of the great nature of CFML allowing you to reduce boilerplate and have cleaner code. At any rate, you may enforce that internally, but good luck getting the same thing out of any 3rd party libs or frameworks.
we tell developers to use local or var for local variables
Well, technically that goes without saying as the ONLY way to declare a local variable is via the “local” scope or “var” keyword so it’s not like you have a choice in that instance.
and THIS scope for variables to share among CFCs.
There’s nothing wrong if that’s your coding guidelines, but that’s not Ortus’ coding guidelines. The variables scope in a CFC has the same lifespan of the “this” scope. The “this” scope is sloppy and unnecessary for private variables as it makes them publicly accessible, so it really breaks best practices. The variables scope should really be used for any private variables that need to persist for the life of the component but are private. The general practice you see in ColdBox is that only 3 scopes are used in 95% of the code:
- arguments
- local
- variables
Variables is used for things like singletons injected into other singletons. And with careful naming guidelines, there’s rarely any ambiguity.
But it doesn’t seem to make sense that ColdBox would have so many unscoped variables.
I don’t understand your confusion. Every variable in Coldbox is in a scope. You seem to be taking issue with the fact that we rely on the well-documented scope-lookup order in CFML which controls how an implicitly scoped variable is referenced. There is nothing wrong with this and it’s been a common practice for as long as CFML has existed.
Why is this not a problem?
Why would it be a problem? You’d need to point out some specific lines of code to get a more specific answer, but every variable in the variables scope inside of ColdBox is placed there on purpose because that’s where it needs to be and referencing it without an explicit scope is a documented behavior of how CFML works. There’s nothing wrong with this. You may prefer to be explicit with scopes in your code, but this is nothing but a style preference. This does not change the behavior of the code, or the garbage collection characteristics of the app.
We’ve had issues with applications filling the heap with objects that can’t be GC’d, and requiring explicitly-scoped ColdFusion variables is one of the things we do to improve that.
Scoping variables will not solve memory leaks unless perhaps the incorrect scope was being used in the first place. I would challenge you to prove how adding an explicit scope to a variable would change garbage collection. I can assure you there are no memory issues due to ColdBox using implicit variable scoping. I’ve been using ColdBox for about 12 years and I’ve been a consultant with Ortus for about 8 years. I’ve analyzed many client heap dumps over the years and solved just as many memory leaks in customers’ apps and never once has a memory issue been in the core of coldbox or even related to scoping for that matter.
Now, lack of var scoping CAN cause bugs in an app if those variables were meant to be local in the first place, But all of the things in the variables scope in ColdBox are there on purpose. Coldbox is used all over the world under very high load and it performs great. The issue with codechecker is it doesn’t have the ability to understand what code is doing and it just blindly tags every variable in the variables scope because it doesn’t know better. It’s up to a human to look at those and determine if they are correct. The varscoper portion of the codechecker tool was written a long time ago and primarily relies on regular expressions. If you want to look for actual var scoping issues, I would recommend the singleton-leak-detector module (on Forgebox). I wrote this to find var scoping issues in client apps and instead of regex-based static code analysis, it does runtime scope analysis of an app in memory. It can still have false positives, but it works much better IMO.