Skip to content

changes lint name: ctypes -> ffi_rust_types #15277

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

Closed
wants to merge 1 commit into from
Closed

changes lint name: ctypes -> ffi_rust_types #15277

wants to merge 1 commit into from

Conversation

Rdbaker
Copy link

@Rdbaker Rdbaker commented Jun 30, 2014

fixes #14704

@emberian
Copy link
Member

emberian commented Jul 1, 2014

Can you squash these commits?

@brson
Copy link
Contributor

brson commented Jul 1, 2014

While I filed the issue, and believe this is a 'better' name, there are some other considerations here:

  • This is a wider problem than just ctypes
  • Because of that, we need to decide if this is a problem we want to solve in general
  • If it is, is this an acceptable incremental improvement that gets us part of the way?
  • If not, is this case bad enough that we want to fix it alone?
  • If we change the name of this lint, what should we do about a transition plan?

Since I've been the most vocal about this, I'd love to hear others' opinions.

@emberian
Copy link
Member

emberian commented Jul 1, 2014

I belive ctypes is the only lint with this problem? I went through the list, and the others seem fine. The only other one which is borderline is type-limits, which when allowed will squelch warnings for useless comparions , so perhaps it can be renamed unnecessary-comparison.

So I don't think this is a wider problem, unless you're refering to something else. I do think this is worth fixing, though. ctypes is a bad name in general. I'm not sure ffi_rust_types is much better, I think I prefer unspecified_repr_in_ffi. It's long, and it should be long, because you're doing really bad things with it. I will revive my patch for struct representation stuff today.

For a transition plan, that's a good point. We could keep in support for ctypes but rather than doing the normal linty thing, check if it is used and emit a deprecation warning saying to use the new name.

@Rdbaker
Copy link
Author

Rdbaker commented Jul 1, 2014

As far as deprecation goes and involving more lints goes, we could keep a map of aliases for different names for each lint (with a 'preferred' name) and if the preferred name is not the one used we could print the deprecation warning. Support for the aliases could be dropped in a later version once appropriate. My only question of this is where this alias map would live. Thoughts on that?

@brson which other lints have this problem?

If we use the map as I suggested we could easily add support for other lints which have this problem which for which an alias would be nice so that a user typing #{allow[ctypes]} wouldn't be shocked that they were given an error due to a non-existent lint

I also agree with @cmr in that another, more explicit name would probably be better. I'm not sure, however, that the name should necessarily be longer, as I don't think doing bad things should necessarily be a pain to do. If anything, I would argue it be shorter so the user is less focused on things like getting the spelling correct and more focused on making sure they're writing safe code.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

@Rdbaker
Copy link
Author

Rdbaker commented Jul 30, 2014

So I understand why it's good to close this RFC, it is very out of date from master. However, I don't understand why, 4 weeks later, my questions were left unanswered. I realize that everybody is busy, but having no comments on an RFC for 4 weeks can be a bit aggravating.

@alexcrichton
Copy link
Member

We try to ensure that the queue keeps moving, but sadly sometimes things fall through the cracks. It's not necessarily anyone's responsibility to ensure that all PRs get proper care and attention, but contributors should always feel free to ping PRs to send out notifications with requests for review every so often to remind reviewers.

I tend to just go through every so often and close PRs that haven't had any activity for more than 10 days, and you really should feel free to reopen with a rebase!

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2023
limit `change_visibility` assist to applicable items

this pr limits the `change_visibility` assist to applicable items. top level items in this context means items that are not nested within `fn`s or `trait`s.

now
```rs
fn foo {
    // assists on this `struct` keyword won't include `change_visibility`
    struct Bar {}
}

trait Foo {
   // same with the `fn` here
   fn bar();
}
```
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.

"ctypes" lint name is confusing
4 participants