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.

Sign up for GitHub

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

Jump to bottom

fix psalm issues #163

Merged
merged 1 commit into from
Mar 23, 2021
Merged

fix psalm issues #163

merged 1 commit into from
Mar 23, 2021

Conversation

azjezz
Copy link
Owner

@azjezz azjezz commented Mar 23, 2021

No description provided.

@azjezz azjezz added Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably ask for additions or changes. Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors. labels Mar 23, 2021
@azjezz azjezz added this to the 1.6.0 milestone Mar 23, 2021
@azjezz azjezz self-assigned this Mar 23, 2021
@azjezz
Copy link
Owner Author

azjezz commented Mar 23, 2021

@muglug

something is weird about this error, it's been present for a while, but never caught in CI, it only happens locally, i assume you encountered this error as well because i can see it was added in your baseline here: https://github.com/muglug/psl/blob/1.6.x/psalm-baseline.xml#L4

image

@coveralls
Copy link

Pull Request Test Coverage Report for Build 681082022

  • 8 of 8 (100.0%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 672045579: 0.0%
Covered Lines: 2922
Relevant Lines: 2922
💛 - Coveralls

azjezz
azjezz commented Mar 23, 2021
View reviewed changes
psalm.xml
Comment on lines +62 to +65

<!-- Not using the result of pure functions is common within PSL -->
<!-- e.g: with and Psl\invariant_violations() -->
<UnusedFunctionCall errorLevel="suppress" />
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this should be considered a bug in psalm or not.

  1. pure function are considered to have no side effects
  2. calling a function with no side effects without using it's results is useless
  3. psalm consider throwing an exception not a side effect
  4. hacklang considers throwing an exception a side effect ( https://github.com/facebook/hhvm/blob/master/hphp/hack/doc/HIPs/contexts_and_coeffects.md )
  5. if throwing an exception is a side effect, this is most likey to result in all functions being non pure, as 90% of calls result in an exception
  6. if throwing an exception is not a side effect, how can a pure function tell psalm that it's okay not to use it's result?

wdyt @muglug?

/cc @Ocramius

@azjezz azjezz merged commit 2f65c9f into 1.6.x Mar 23, 2021
@azjezz azjezz deleted the fix/typing branch March 23, 2021 21:59
@azjezz azjezz mentioned this pull request Mar 23, 2021
Merged
@muglug
Copy link

muglug commented Mar 23, 2021

Do you think UnusedVarAnnotation should be a level 1 Psalm issue? Maybe I should backtrack and only make UnusedVariable (which can have other ramifications) a level 1 issue

@azjezz
Copy link
Owner Author

azjezz commented Mar 23, 2021

I think UnusedVarAnnotation should be a level 7 issue, it's easy to solve.
UnusedVariable on the other hand should be a level 1 issue, the change would be to add _ as a prefix to the variable name, and people might not want to bother with it if they just added psalm to their codebase.

I previously added those @var annotations because psalm wasn't detecting the type correctly, so it makes sense to remove them.

@muglug
Copy link

muglug commented Mar 24, 2021

Yeah, I guess people can always suppress them

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

@azjezz azjezz

Labels
Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: In Progress This issue is being worked on, and has someone assigned. Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@azjezz @coveralls @muglug