Skip to content

Implemented option_option lint #2299

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

Merged
merged 8 commits into from Jan 19, 2018
Merged

Implemented option_option lint #2299

merged 8 commits into from Jan 19, 2018

Conversation

ghost
Copy link

@ghost ghost commented Dec 26, 2017

This implements the lint introduced in issue #97

cx,
OPTION_OPTION,
ast_ty.span,
"consider using `Option<T>` instead of `Option<Option<T>>`",
Copy link
Contributor

Choose a reason for hiding this comment

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

... "or a custom enum if you need to distinguish all 3 cases"

OPTION_OPTION,
ast_ty.span,
"consider using `Option<T>` instead of `Option<Option<T>>`",
"`Option<_>` is easier to use than `Option<Option<_>`",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs the additional help. Just use span_lint

@ghost
Copy link
Author

ghost commented Jan 17, 2018

I made the requested changes.

output_nested();

// The lint allows this
let local: Option<Option<u8>> = None;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why this isn't linted?

Copy link
Author

Choose a reason for hiding this comment

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

Based on the discussion in #97. I thought the lint shouldn't apply to local bindings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked through the discussion and could not find anything. Can you quote what you mean?

Copy link
Author

Choose a reason for hiding this comment

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

#97 (comment)
"It should catch Option<Option<..>> types in function signatures and perhaps typedefs."

I took this to mean only function signatures, struct definitions and enum definitions. I might have been incorrect.

In any case, I think it would be strange to allow the type in expressions but not allow naming those expressions local bindings. Maybe we should move this discussion back into the issue if you want to discuss further.

Copy link
Contributor

Choose a reason for hiding this comment

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

Being conservative is fine here.

Michael Wright added 5 commits January 18, 2018 07:21
Rename `vec` to `ty` in `match_type_parameter`. This variable is a type
and not a vector. Previously it would only refer to `Vec<_>` so the name used
to make sense.
@oli-obk oli-obk merged commit 8a53d90 into rust-lang:master Jan 19, 2018
@ghost ghost deleted the option_option_pr branch January 19, 2018 17:52
@felix91gr felix91gr mentioned this pull request Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant