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

Cognitive Complexity PR, re-gitted #4516

Closed
wants to merge 4 commits into from

Conversation

felix91gr
Copy link
Contributor

@felix91gr felix91gr commented Sep 6, 2019

A continuation of the work done in #3963 due to git conflicts.

References:

Overall todo:

  • Remove the ICE described by Manish.

  • Decide if and how to score the constructs that are not being scored right now at all (most of them were not considered in the paper and so I thought that maybe there could be a default, low value, for non-specified kinds of expressions. Either do that, or just skip them for now and expand upon them in later versions).

  • Roll back the implementation of flexible scores (done in one of the last commits of the original PR). This is because we want to keep this PR as simple as possible, and steering too far out of where the paper scores things will make future expansions harder to do in the right direction. For now, the minimal, the better.

  • Write the complete specification for what's been implemented (the Draft should be a good base, and the original paper should be used as a reference). This should end up in the docs directory of Clippy, I think.

  • Finish writing up test cases and tidy up the test folder for this lint.

Overall already done:

  • Implemented the entire spec described in the Draft.
  • Changed Lint from Late to Early pass.
  • Downgraded Lint to Nursery (in preparation for next step) (thanks Manish!)

@felix91gr
Copy link
Contributor Author

@vityafx I think this is much better now as a base. You should be able to base your work on it.

It doesn't pass the dogfood test (which is basically Clippy linting itself) but that's okay, I should be able to fix those soon. The code is, or should be, pretty much sound already.

@felix91gr
Copy link
Contributor Author

@Manishearth there's a new ExprKind, Let. You also left a comment for me about the if let statements:

XXXManishearth don't count complexity of if let
(For the IF-Case)

I'm guessing they're now being treated differently in the AST? What were the changes made to the If nodes, and what do the Let nodes cover?

(iirc there was work being done in the syntax so that one could chain if-lets together or something, right? Do you happen to know which RFC it was, and if it is that one that caused the change?)

@Manishearth
Copy link
Member

Yeah so now if let is represented as an If block with a Let condition

@felix91gr
Copy link
Contributor Author

Ohh. And this is the documentation for ExprKind::Let as well:

A let pat = expr expression that is only semantically allowed in the condition of if / while expressions. (e.g., if let 0 = x { .. }).

So, that ExprKind is only generated in the context of an if let or while let. Right?

@Manishearth
Copy link
Member

yep

@felix91gr
Copy link
Contributor Author

Awesome, Manish, thank you.

@felix91gr
Copy link
Contributor Author

Btw I talked it over with Oliver and we've decided to keep this as close to the original paper as possible. Therefore syntax nodes that aren't sufficiently described there will not score at all. This is so as to make this PR as simple as possible, and to facilitate future expansions upon the lint.

@bors
Copy link
Contributor

bors commented Sep 8, 2019

☔ The latest upstream changes (presumably #4523) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995 flip1995 added C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 11, 2019
@felix91gr
Copy link
Contributor Author

Please go here to the lint's discussion, to see why I'm closing the PR 🙂

@felix91gr felix91gr closed this Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants