Rule UNUSED_LOCAL_VARIABLE has issues with several tags #339

HarryMuc · 2017-07-06T11:16:40Z

CFLint return the error message: local variable X is not used in function Y

But the local variables are used in tags like cfloop, cflocation, cfcookie, ...:

<cfset var lLoginURLParams = stContens.security.lLoginURLParams>
    <cfloop list="#lLoginURLParams#" index="sParam">

<cfset var sCoreURL = stContens.sCoreURL>
    <cflocation url="#sCoreURL#contens/index.cfm#sLoginParams#" addtoken="No">

<cfset var sRememberUserCookieKey = getRememberUserLoginCookieName()>
    <cfcookie name="#sRememberUserCookieKey#" value="#sCookieValue#" expires="#sExpirePeriod#">

The text was updated successfully, but these errors were encountered:

ryaneberly · 2017-07-17T02:14:50Z

These are implemented. Thanks for reporting it.

HarryMuc · 2017-07-17T06:45:47Z

Thanks a lot Ryan! Will test asap

HarryMuc · 2017-07-17T09:47:54Z

Sorry, this issue is not fixed with 1.2.0 - I still have lots of false positives
e.g.

<cfset var qWork = "">
...
<cfquery name="qWork" datasource="#variables.sPublishDataSource#">
...
</cfquery>

fixed.

ryaneberly · 2017-07-18T01:33:58Z

I'm assuming you meant:

<cfset var sPublishDataSource= "">
...
<cfquery name="qWork" datasource="#variables.sPublishDataSource#">
...
</cfquery>

I added @Datasource to the expressions that are considered, and fixed an issue with the variables scope.

HarryMuc · 2017-07-18T07:48:49Z

No, I didn't mean the datasource attribute - the name attribute is important.
We always local scope the query name, also the loop index ...

atm the rule returns false positives for:

LOOP

<cfset var iIdx = "">
<cfloop list="#arguments.lID#" index="iIdx">...</cfloop>

<cfset var ifolder = 0>
<cfloop collection="#attributes.foldermapping#" item="ifolder">...</cfloop>

QUERY

<cfset var qAllPages = "">
<cfquery name="qAllPages" datasource="#variables.sPublishDataSource#">...</cfquery>

LOCAL

<cfset var local = {}>
<cfset local.stEditor = ...>

ryaneberly · 2017-07-18T12:03:44Z

@derrick13 ,
Thanks, are you saying that when the query name is declared, we should consider that variable used?

I would expect this to flag unused:

<cfset var qAllPages = "">
<cfquery name="qAllPages" datasource="#variables.sPublishDataSource#">...</cfquery>

and this to NOT flag it:

<cfset var qAllPages = "">
<cfquery name="qAllPages" datasource="#variables.sPublishDataSource#">...</cfquery>
<cfloop query="qAllPages">...</cfloop>

Am I missing something?

HarryMuc · 2017-07-18T13:01:02Z

Hi Ryan, see https://luceeserver.atlassian.net/browse/LDEV-1130 One of the more poor design decisions in ColdFusion was to have the 'scopes' created by tags such as cfcatch cffile, and cfhttp (and for that matter, the name attribute of cfquery) go into the variables scope by default when called from the context of a method. I've seen a lot of code over the years with hard-to-detect bugs due to values bleeding into the variables scope. Also cfloop index and item and other attributes.

ryaneberly · 2017-07-18T13:30:21Z

I agree. The varscoper encourages this. I'm just wondering, if any of these variables aren't actually dereferenced, do we care? Or should we just ignore them all?

…

On Tue, Jul 18, 2017 at 9:07 AM, Harry Klein ***@***.***> wrote: Hi Ryan, see https://luceeserver.atlassian.net/browse/LDEV-1130 One of the more poor design decisions in ColdFusion was to have the 'scopes' created by tags such as cfcatch cffile, and cfhttp (and for that matter, the name attribute of cfquery) go into the variables scope by default when called from the context of a method. I've seen a lot of code over the years with hard-to-detect bugs due to values bleeding into the variables scope. Also cfloop index and item and other attributes. Von: ryaneberly ***@***.*** Gesendet: Dienstag, 18. Juli 2017 14:04 An: cflint/CFLint ***@***.***> Cc: Harry Klein ***@***.***>; Mention ***@***.***> Betreff: Re: [cflint/CFLint] Rule UNUSED_LOCAL_VARIABLE has issues with several tags (#339) @derrick13<https://github.com/derrick13> , Thanks, are you saying that when the query name is declared, we should consider that variable used? I would expect this to flag unused: <cfset var qAllPages = ""> <cfquery name="qAllPages" datasource="#variables. sPublishDataSource#">...</cfquery> and this to NOT flag it: <cfset var qAllPages = ""> <cfquery name="qAllPages" datasource="#variables. sPublishDataSource#">...</cfquery> <cfloop query="qAllPages">...</cfloop> Am I missing something? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://github.com/ cflint/CFLint#339#issuecomment-316043208>, or mute the thread< https://github.com/notifications/unsubscribe-auth/ AAWkYHkpnCPq3YD36etKEHVwSJZ1Ls7Aks5sPJ8igaJpZM4OPfpx>. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#339 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFtZu2QTibZXN7NPdV35f_Iiq3lYKsKfks5sPKyRgaJpZM4OPfpx> .

HarryMuc · 2017-07-18T14:17:43Z

Imho we could just ignore them all

TheRealAgentK · 2017-07-18T21:35:29Z

@ryaneberly @derrick13

As opposed to Ryan, I personally would treat both cases below as NOT flagging UNUSED_LOCAL_VARIABLE. To me it doesn't matter if you use qAllPages in a loop or output or whatever after the query - the mere fact that it's used as a name for the query should be enough to not kick off this rule.

<cfset var qAllPages = "">
<cfquery name="qAllPages" datasource="#variables.sPublishDataSource#">...</cfquery>

<cfset var qAllPages = "">
<cfquery name="qAllPages" datasource="#variables.sPublishDataSource#">...</cfquery>
<cfloop query="qAllPages">...</cfloop>

ryaneberly · 2017-07-18T21:52:49Z

@TheRealAgentK, I'm good with that. Don't want to ding someone for using a best practice that makes up for a coldfusion deficiency.

ryaneberly · 2017-07-18T22:16:21Z

@derrick13 ,
I took care of cfquery/@name, cfloop/@Index, and cfloop/@item.. Help with an exhaustive list would be great.

Still looking at the local struct issue.

TheRealAgentK · 2017-07-18T22:35:40Z

Here's some - but it'd be good if @derrick13 could verify those first.

cfchart / name
cfdocument / name
cfftp / name when action = "listDir"
cfhtmltopdf / name
cfhttp / resultname
cfimage / name
cfimap / name when action GetAll, GetHeaderOnly or ListAllFolders
cfldap / name when action Query
cfoutput / query
cfpdf / name
cfreport / name
cfsavecontent / name
cfstoreproc / result
cfxml / variable

also: this is just what I could easily think of - most likely not exhaustive.

fix local false positive

ryaneberly · 2017-07-19T02:21:02Z

@derrick13
The example that uses

<cfset var local = {}>
<cfset local.stEditor = ...>

is an interesting one. It was a false positive because local is a scope. The false positive is removed. However, it seems to me that initializing local is bad practice now that local is a scope. Thoughts?

ryaneberly · 2017-07-19T02:21:35Z

@TheRealAgentK

Your list is implemented. It is parameterized in cflint.definition.json.

fix test

HarryMuc · 2017-07-19T08:03:59Z

Wow, thanks for your work Ryan - and for the list Kai!
The rule now works better and better!
I waded through a lot of files and it is almost perfect now.
Only these 3 false positives where still found:

var scoped method used in query

	<cfset var oQueryFct = super.getAppHelper().instanceFactory("contenscms.util.system.queryfct")>

	<cfquery datasource="#variables.stContens.sDatasource#">
	SELECT ...
	WHERE #oQueryFct.cutIDlist("accessdata_ID", arguments.idList)#
	</cfquery>

var scoped variable used in query

	<cfset var iThisServerId = oServersDatamanager.findIdByServername(servername=variables.stContens.sServerName)>

	<cfquery name="qConnections" datasource="#variables.stContens.sDataSource#" maxrows="1">
	SELECT	...
	WHERE	co_servers_connections_rel.server_ID = #iThisServerId#
	</cfquery>

var scoped cfheader file attribute

	<cfset var sFile = "#variables.sCmfAppDir#cmf-#arguments.sModuleFolder#.zip">

	<cfheader name="content-disposition" value="inline; filename=cmf-#arguments.sModuleFolder#.zip"
	><cfcontent type="application/x-zip-compressed" file="#sFile#" deletefile="No" reset="yes"
	><cfabort>

HarryMuc · 2017-07-19T08:09:08Z

Found another one:
4) var scoped variable in cflog text

<cfset var sSecureFolder = expandPath("/#variables.stContens.applicationname#/_secure/collections/")>

<cflog file="content_errors" text="Please check the lucene collections in the folder #sSecureFolder# and manually remove the file 'write.lock'! #cfcatch.message#">

HarryMuc · 2017-07-19T08:43:10Z

@ryaneberly "... it seems to me that initializing local is bad practice now that local is a scope ..."

Definitely, it is bad practice now but some of our code is old and was written before ACF 9 (which introduced the local scope)

See also:
http://forta.com/blog/index.cfm/2009/6/21/The-New-ColdFusion-LOCAL-Scope
http://blog.adamcameron.me/2013/12/34-dumb-things-coldfusion-does.html
http://www.carehart.org/blog/client/index.cfm/2010/3/4/resources_on_the_var_scope_problem

ryaneberly · 2017-07-19T12:01:19Z

@derrick13 would it be valuable to add a different linting rule for the declaration of a "local" variable?

HarryMuc · 2017-07-19T12:07:12Z

imho it would just complicate the configuration, I wouldn't add a new rule

ryaneberly · 2017-07-20T02:00:45Z

@derrick13, thanks for the detailed research. Much appreciated.

1 &2 - parsing cfquery is dicey -- suggest 1.3.0
3 - cfparser needs to support cfcontent cfparser/cfparser#81
4 - I can't duplicate

HarryMuc · 2017-07-20T07:26:14Z

I guess 1-4 are all related to cfparser - not sure if it would be possible to use the integrated Lucee parser instead of cfparser?
4 - cfparser needs to support cflog text parsing?

HarryMuc · 2017-07-20T07:30:36Z

Issue
Severity:INFO
Message code:UNUSED_LOCAL_VARIABLE
	File:D:\projekte_cf\contens\contens5\contens\core\scheduler\systemscheduler.cfc
	Column:0
	Line:354
		Message:Local variable sSecureFolder is not used in function checkCollections, consider removing it.
		Variable:'sSecureFolder' in function: checkCollections
		Expression:<cffunction name="checkCollections" access="public" returntype="struct" output="No" hint="checks collections">
		<cfset var sSecureFolder = expandPath("/#variables.stContens.applicationname#/_secure/


Total files:1
Total size 1227

Issue counts:1
UNUSED_LOCAL_VARIABLE:1

Total issues:1

HarryMuc · 2017-07-20T07:31:42Z

	<cffunction name="checkCollections" access="public" returntype="struct" output="No" hint="checks collections">
		<cfset var sSecureFolder = expandPath("/#variables.stContens.applicationname#/_secure/collections/")>
		<cfset var stReturn = {}>
		<cfset var stResult = {}>
		<cfset var oAgents = getAppHelper().instanceFactory("contenscms.core.search.agent")>
		<cfset var lAgents = oAgents.getAgents()>
		<cfset var sAgent = "">
		<cfset var oAgent = "">

		<cfset stReturn.ok = true>
		<cfset stReturn.message = ''>
		<cfset stReturn.errormessage = ''>
		<cfset stReturn.data = {}>

		<cftry>
			<cfloop list="#lAgents#" index="sAgent">
				<!--- prevent parse errors --->
				<cflock name="checkCollection" type="exclusive" timeout="10">
					<cfset stResult = {}>

					<cfset oAgent = oAgents.getSearchAgent(sAgent)>

					<cfif isObject(oAgent)>
						<cfset stResult = oAgent.search(sTerm="dummytermtocheckforlocks", num=1)>
					</cfif>

					<cfset stReturn.data[sAgent] = stResult>

					<cfif stResult.ok>
						<cfset stReturn.message = 'Checked collections successfully.'>
					<cfelse>
						<cfset stReturn.ok = false>
						<cfset stReturn.errormessage = 'Error in Collection' & " > " & sAgent & " ">
					</cfif>
				</cflock>
			</cfloop>

			<cfcatch>
				<cflog file="content_errors" text="Please check the lucene collections in the folder #sSecureFolder# and manually remove the file 'write.lock'! #cfcatch.message#">
				<cfset stReturn.ok = false>
				<cfset stReturn.errormessage = cfcatch.message>
				<cfset stReturn.data = cfcatch>
			</cfcatch>
		</cftry>

		<cfif NOT stReturn.ok>
			<cfset stReturn.message = stReturn.errormessage>

			<!--- notify users about invalid collections --->
			<cfif structKeyExists(variables.stContens.search, "agentnotifications")>
				<cftry>
					<cfmail to="#variables.stContens.search.agentnotifications.notificationmailaddresses#" charset="UTF-8" from="#variables.stContens.search.agentnotifications.notificationmailsender#" subject="Agent notification: '#application.applicationname#'" timeout="30">Invalid search collection(s), switched to database fallback mode! Agent: #sAgent#</cfmail>
					<cfcatch></cfcatch>
				</cftry>
			</cfif>
		</cfif>

		<cfreturn stReturn>
	</cffunction>

denuno · 2017-07-21T04:16:43Z

As far as using an engine's parser:

Engine parsers (like Lucee) have a different set of concerns than linters and IDEs have, which basically rules them out, as they're pure runtime (id est, one error at a time), while we (linters/IDE parsers) have to kind of swallow errors and keep on chugging to be at all useful.

That, and we want to know information that the engine parser doesn't care about-- variable names, what's scoped where, etc., etc..

ryaneberly · 2017-07-28T01:36:06Z

@derrick13
I can't duplicate the issue with cflog text even with your full source example. It may be related to my local version of cfparser.

Does it work for you now that the dev trunk is using cfparser 2.4.10?

HarryMuc · 2017-07-28T06:41:18Z

@ryaneberly I updated my version and now the log issue is gone.
Thanks for updating the dev trunk with the new cfparser version!

ryaneberly · 2017-07-28T12:41:24Z

@derrick13 Thanks for confirming.

TheRealAgentK added the bug label Jul 6, 2017

ryaneberly added a commit that referenced this issue Jul 17, 2017

#339 fixed

71bc823

ryaneberly self-assigned this Jul 17, 2017

ryaneberly added this to the 1.2.0 milestone Jul 17, 2017

ryaneberly closed this as completed Jul 17, 2017

ryaneberly added a commit that referenced this issue Jul 18, 2017

#339

14b863a

fixed.

ryaneberly reopened this Jul 18, 2017

ryaneberly added a commit that referenced this issue Jul 18, 2017

#339

8bfefb9

ryaneberly added a commit that referenced this issue Jul 19, 2017

#339

c86009f

ryaneberly added a commit that referenced this issue Jul 19, 2017

#339

ffd72f2

fix local false positive

ryaneberly added a commit that referenced this issue Jul 19, 2017

#339

3334aee

fix test

ryaneberly added a commit that referenced this issue Jul 20, 2017

#339

4d4c60d

ryaneberly closed this as completed Jul 28, 2017

ryaneberly mentioned this issue Aug 9, 2017

MISSING_VAR in CFLOOP index #413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rule UNUSED_LOCAL_VARIABLE has issues with several tags #339

Rule UNUSED_LOCAL_VARIABLE has issues with several tags #339

HarryMuc commented Jul 6, 2017 •

edited

Loading

ryaneberly commented Jul 17, 2017

HarryMuc commented Jul 17, 2017

HarryMuc commented Jul 17, 2017 •

edited

Loading

ryaneberly commented Jul 18, 2017

HarryMuc commented Jul 18, 2017 •

edited

Loading

ryaneberly commented Jul 18, 2017 •

edited

Loading

HarryMuc commented Jul 18, 2017 via email •

edited

Loading

ryaneberly commented Jul 18, 2017 via email

HarryMuc commented Jul 18, 2017 via email •

edited

Loading

TheRealAgentK commented Jul 18, 2017

ryaneberly commented Jul 18, 2017

ryaneberly commented Jul 18, 2017

TheRealAgentK commented Jul 18, 2017 •

edited

Loading

ryaneberly commented Jul 19, 2017

ryaneberly commented Jul 19, 2017

HarryMuc commented Jul 19, 2017

HarryMuc commented Jul 19, 2017

HarryMuc commented Jul 19, 2017

ryaneberly commented Jul 19, 2017

HarryMuc commented Jul 19, 2017

ryaneberly commented Jul 20, 2017

HarryMuc commented Jul 20, 2017

HarryMuc commented Jul 20, 2017

HarryMuc commented Jul 20, 2017

denuno commented Jul 21, 2017

ryaneberly commented Jul 28, 2017

HarryMuc commented Jul 28, 2017

ryaneberly commented Jul 28, 2017

Rule UNUSED_LOCAL_VARIABLE has issues with several tags #339

Rule UNUSED_LOCAL_VARIABLE has issues with several tags #339

Comments

HarryMuc commented Jul 6, 2017 • edited Loading

ryaneberly commented Jul 17, 2017

HarryMuc commented Jul 17, 2017

HarryMuc commented Jul 17, 2017 • edited Loading

ryaneberly commented Jul 18, 2017

HarryMuc commented Jul 18, 2017 • edited Loading

ryaneberly commented Jul 18, 2017 • edited Loading

HarryMuc commented Jul 18, 2017 via email • edited Loading

ryaneberly commented Jul 18, 2017 via email

HarryMuc commented Jul 18, 2017 via email • edited Loading

TheRealAgentK commented Jul 18, 2017

ryaneberly commented Jul 18, 2017

ryaneberly commented Jul 18, 2017

TheRealAgentK commented Jul 18, 2017 • edited Loading

ryaneberly commented Jul 19, 2017

ryaneberly commented Jul 19, 2017

HarryMuc commented Jul 19, 2017

HarryMuc commented Jul 19, 2017

HarryMuc commented Jul 19, 2017

ryaneberly commented Jul 19, 2017

HarryMuc commented Jul 19, 2017

ryaneberly commented Jul 20, 2017

HarryMuc commented Jul 20, 2017

HarryMuc commented Jul 20, 2017

HarryMuc commented Jul 20, 2017

denuno commented Jul 21, 2017

ryaneberly commented Jul 28, 2017

HarryMuc commented Jul 28, 2017

ryaneberly commented Jul 28, 2017

HarryMuc commented Jul 6, 2017 •

edited

Loading

HarryMuc commented Jul 17, 2017 •

edited

Loading

HarryMuc commented Jul 18, 2017 •

edited

Loading

ryaneberly commented Jul 18, 2017 •

edited

Loading

HarryMuc commented Jul 18, 2017 via email •

edited

Loading

HarryMuc commented Jul 18, 2017 via email •

edited

Loading

TheRealAgentK commented Jul 18, 2017 •

edited

Loading