Allow classes/functions from a later version if they are used conditionally
ClosedPublic
Actions

Authored by joshuaspence on Aug 3 2014, 12:08 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T5299: `ArcanistXHPASTLinter` should allow classes/functions from a later version if they are used conditionally

Commits

rARC3f79ae258f34: Allow classes/functions from a later version if they are used conditionally

Summary

Fixes T5299. Currently, linting src/future/http/HTTPSFuture.php from rPHU will raise the following error:

Error  (XHP31) Use Of PHP 5.3 Features
 This codebase targets PHP 5.2.3, but `curlfile` was not introduced until
 PHP 5.5.0.

          532       // use this "@" stuff.
          533 
          534       if (class_exists('CURLFile')) {
 >>>      535         $file_value = new CURLFile((string)$tmp, $info['mime'], $info['name']);
          536       } else {
          537         $file_value = '@'.(string)$tmp;
          538       }

However, since this class is being used conditionally, it should be fine and no linter errors should be being raised.

Test Plan

Added a test case.

Diff Detail

Repository

rARC Arcanist

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

joshuaspence updated this revision to Diff 24370.Aug 3 2014, 12:08 PM

joshuaspence retitled this revision from to Allow classes/functions from a later version if they are used conditionally.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

joshuaspence added a task: T5299: `ArcanistXHPASTLinter` should allow classes/functions from a later version if they are used conditionally.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptAug 3 2014, 12:08 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B2015: Diff 24370.Aug 3 2014, 12:09 PM

joshuaspence updated this object.Aug 3 2014, 12:09 PM

joshuaspence edited edge metadata.

This still needs a little bit of work and I want to try to cover more complex test cases such as:

if (function_exists('foo') && foo()) {}

if (function_exists('foo') && function_exists('bar')) {
  foo();
  bar();
}

I don't think it's necessarily worthwhile to catch those complex cases -- the "unknown symbol" message could maybe just hint that you need to put the guard call in its own conditional. At most, I'd probably just raise an alternate warning: "unknown symbol in block which looks like it might guard it but is too complex to be statically analyzed".

I think using token IDs (not node IDs) will work better for the range stuff, though. See inlines.

This looks great otherwise.

src/lint/linter/ArcanistXHPASTLinter.php
422–430	Maybe find the `n_STATEMENT_LIST` of the conditional body, call `getTokens()` on it, and take the first and last keys instead. (There might be getFirstToken() / getLastToken() methods or something like that, too?) I'm not 100% sure that node IDs are "sequential" in a way that isn't surprising, because they form a tree. Token IDs are flat and sequential, so using them might be easier and avoid eventual issues.
445	Or use the first key of $node->getTokens().