Static Analysis Results #933

Closed
rlerdorf opened this Issue May 11, 2013 · 4 comments

3 participants

@rlerdorf

The static analyzer really hates all the extract() stuff everywhere, but filtering those, left a couple of valid-looking issues:

--------------------------------
File       : console/Command.php:264
Reason     : TooManyArgument
Snippet    : $this->out($text, 1, 'heading')
Github     : https://github.com/UnionOfRAD/lithium/blob/master/console/Command.php#L264
--------------------------------
File       : util/Set.php:587
Reason     : RequiredAfterOptionalParam
Snippet    : $data = array()
Github     : https://github.com/UnionOfRAD/lithium/blob/master/util/Set.php#L587
--------------------------------
File       : data/collection/MultiKeyRecordSet.php:94
Reason     : TooManyArgument
Snippet    : $this->_populate($offset)
Github     : https://github.com/UnionOfRAD/lithium/blob/master/data/collection/MultiKeyRecordSet.php#L94
--------------------------------
File       : net/http/Media.php:673
Reason     : BadPassByReference
Snippet    : (array)$types[$type]
Github     : https://github.com/UnionOfRAD/lithium/blob/master/net/http/Media.php#L673
@nateabele
Union of RAD member

The extract() stuff is the result of a particular pattern we use (wherein the effects are always transparent), but I thought I actually removed most of them. In either case, thanks for the report, I'll go through these shortly. Forgive my ignorance, but what are you using to do the analysis?

@rlerdorf

It is just hhvm --hphp -t analyze

@davidpersson
Union of RAD member

Referenced by 7b1f156.

@davidpersson davidpersson added a commit to davidpersson/lithium that referenced this issue May 3, 2014
@davidpersson davidpersson Removing `extract()` usage where it makes sense.
References #933.

- Refactoring Libraries::_replaceAfterExtract to not use extract.
- Updating docblocks.
6eeb646
@davidpersson davidpersson added a commit to davidpersson/lithium that referenced this issue May 3, 2014
@davidpersson davidpersson Fixing TooManyArgument and BadPassByReference warnings.
Refs #933.
1d73556
@davidpersson davidpersson added a commit to davidpersson/lithium that referenced this issue May 3, 2014
@davidpersson davidpersson Fixing TooManyArgument and BadPassByReference warnings.
Refs #933.

- `_populate()` does not take an offset as an argument.
  The callee (`MultiKeyRecordSet::offsetGet()`) will work
  as intended nonetheless.

- Assign to a variable and cast before passing to `current()`.
e357aec
@davidpersson davidpersson added a commit to davidpersson/lithium that referenced this issue May 3, 2014
@davidpersson davidpersson Removing `extract()` usage where only minor changes are needed.
References #933.
726a5cd
@davidpersson davidpersson added a commit that referenced this issue May 3, 2014
@davidpersson davidpersson Fixing HHVM's `TooManyArgument` and `BadPassByReference` warnings.
Refs #933.

- `_populate()` does not take an offset as an argument.
  The callee (`MultiKeyRecordSet::offsetGet()`) will work
  as intended nonetheless.

- Assign to a variable and cast before passing to `current()`.
0de8fa0
@davidpersson davidpersson added a commit that referenced this issue May 3, 2014
@davidpersson davidpersson Removing `extract()` usage where only minor changes are needed.
In following cases `extract()` was not removed:

- Many variables are extracted. Removing would cause many variables to
  be rewritten making the code significantly uglier.
- The concept of a method relies on extract(). I.e. overwriting etc.
- It seemed not safe to remove it as it was hard to trace back which
  variables are actually extracted.

References #933.
651d07a
@davidpersson
Union of RAD member

Most parts where extract() was used have now been refactored. See the commit message of 651d07a for reasons why some were left.

The 4 issues you've pointed out have subsequently been fixed in 0de8fa0, 9ddd564 and 7b1f156.

I've had a hard time getting HHVM setup on my machines. As the analyzer seems quite powerful I really would have liked to run it on my own. Will try to get it setup again soon. In the meantime if anybody with HHVM would like to run the analyzer against the codebase - that would be appreciated.

Thanks for reporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment