Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New MISSING_VAR false positives in 1.3.0 around cfscript/tag syntax #517

Closed
TheRealAgentK opened this issue Jan 1, 2018 · 13 comments
Closed
Assignees
Milestone

Comments

@TheRealAgentK
Copy link
Collaborator

TheRealAgentK commented Jan 1, 2018

ACF, here are a few examples of new false positives (150+ in my codebase) I just found :-/

They never triggered false positives in 1.2.x. but do now in 1.3+

var qGetSites = new Query(datasource="...", sql="...");

Variable datasource is not declared with a var statement, same for "variable" sql.

var mailService=new mail(
                    to="...",
                    from="...",
                    subject="...",
                    type="HTML",
                    body=mailBody
                    );

all 5 valid attributes here trigger now MISSING_VAR

var httpRequest = new HTTP(
        method = "get",
        url = ("http://api.pusherapp.com" & resourceUri),
        charset = "utf-8"
            );

method and charset trigger a MISSING_VAR, url doesn't. No idea why.

var httpRequest = new Http(method=arguments.method, url=variables.ENDPOINT & arguments.resourceUri, charset="utf-8", timeout=arguments.timeout, throwOnError=false, username=variables.something, password=variables.somethingelse, getAsBinary="never");

charset, throwOnError, username, password, getAsBinary trigger MISSING_VAR

application.facebookSDK.facebookApp = new facebook.sdk.FacebookApp(appId=application.config.FBPhotoEndpoint_APPID, secretKey=application.config.FBPhotoEndpoint_SECRET_KEY, apiVersion=application.config.FBPhotoEndpoint_Version);

Reports MISSING_VAR on each of the constructor arguments appId, secretKey, apiVersion

void function setCallerVars(data, row, getRow, columnNames){
        var rowData = getRow(data, row, columnNames);
        for (var col in listToArray(columnNames)){
        	if (isQuery(data) && ListFindNoCase(data.columnList, col)) {
				caller[col] = rowData[col];
        	}
        }
        caller["currentRow"] = currentRow;
    }

The above is in a custom tag. Reports on "caller" for MISSING_VAR. "Caller" is a scope similar to thisTag in a custom tag and should then not report MISSING_VAR.

@TheRealAgentK TheRealAgentK added this to the 1.3.1 milestone Jan 1, 2018
@TheRealAgentK
Copy link
Collaborator Author

I remember having seen a ticket that dealt with those built-in CFML objects for 1.3.0 - any chance whoever worked on it could have a look into these issues?

@KamasamaK
Copy link
Collaborator

There was #482 and #483 which seem similar.

@TheRealAgentK
Copy link
Collaborator Author

Yeah, it seems that something related to the fixes for either of these tickets now creates the issues with the "new" syntax.

@ryaneberly
Copy link
Contributor

Sounds like the improved parsing is causing more false positives.

@timbeadle
Copy link

👍 I've seen this issue as well.

@ryaneberly
Copy link
Contributor

ryaneberly added a commit that referenced this issue Jan 11, 2018
ryaneberly added a commit that referenced this issue Jan 11, 2018
@ryaneberly
Copy link
Contributor

ryaneberly commented Jan 11, 2018

@TheRealAgentK, are you getting the false positives on the dev branch? I'm not aware of any fixes, but the tests I added to
2b4a9aa
based on your examples do not generate false positives.
(other than the caller scope example, which I fixed)

@TheRealAgentK
Copy link
Collaborator Author

Pretty sure I got the issue with a local build of the dev branch. Your tests look like my scenarios - I'll look closer into that later today or on the weekend - weird.

@lmajano
Copy link

lmajano commented Feb 15, 2018

Yep, this is 1.3.0 and here are some more code samples if needed:

var oExpectation = new Expectation( spec=this, assertions=this.$assert, mockbox=this.$mockbox );

// Interesting one
thread.closures = arguments.closures;

@ryaneberly
Copy link
Contributor

ryaneberly commented May 5, 2018

Hi @lmajano
Can you provide a complete example. I do not get the problem on:

component{
  function foo(){
    var oExpectation = new Expectation( spec=this, assertions=this.$assert, mockbox=this.$mockbox );
    thread.closures = arguments.closures;
  } 
} 

ryaneberly added a commit that referenced this issue May 5, 2018
@KamasamaK
Copy link
Collaborator

@ryaneberly I am able to reproduce the issue on 1.3.0 as it is. However, it appears to be fine on the latest dev snapshot.

@ryaneberly
Copy link
Contributor

Interesting. I downloaded the 1.3.0 from maven and couldn't reproduce it in isolation. Thanks for checking.

ryaneberly added a commit that referenced this issue May 6, 2018
* #517 added caller and thisTag scopes

* tests for #517

* #517
@ryaneberly
Copy link
Contributor

I think this can be closed and included in 1.4.0

@ryaneberly ryaneberly modified the milestones: 1.3.1, 1.4.0 May 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants