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

Fix item visibility conversion for rustc #26

Closed
2 tasks
xFrednet opened this issue Jun 30, 2022 · 7 comments · Fixed by #294
Closed
2 tasks

Fix item visibility conversion for rustc #26

xFrednet opened this issue Jun 30, 2022 · 7 comments · Fixed by #294
Assignees
Labels
A-api Area: Stable API C-bug Category: Something isn't working D-rustc-driver Driver: Rustc Driver E-help-wanted Participation: Issues with some complexity and where help would be highly appreciated
Milestone

Comments

@xFrednet
Copy link
Member

xFrednet commented Jun 30, 2022

With nightly-2022-06-30 rustc's internal visibility representation changed to a simple enum that holds three vales. See https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.Visibility.html

This is an interesting and perhaps a nicer representation that the one in linter_api. For the sync I only temporarily fixed the conversion for rustc since it was already not working and every situation.

This issue has two tasks:

  1. decide an API representation for visibility (See also Rust Reference)
  2. fix the conversion in linter_driver_rustc
@xFrednet xFrednet added C-bug Category: Something isn't working E-help-wanted Participation: Issues with some complexity and where help would be highly appreciated A-api Area: Stable API D-rustc-driver Driver: Rustc Driver labels Jun 30, 2022
bors bot added a commit that referenced this issue Jun 30, 2022
27: Rustup -> nightly-2022-06-30 r=xFrednet a=xFrednet

This update is sadly not super clean, but I've crated an issue #26 for the introduced bug.

Co-authored-by: xFrednet <xFrednet@gmail.com>
@DevinR528
Copy link
Contributor

This is a perfect example of what we are trying for. We need to make sure this change can be made without a semver break to the linter_api crate. Obviously it's still early enough that we are fine breaking compatibility but we should keep examples like this in mind.

@xFrednet
Copy link
Member Author

Exactly, that's why I've also not fixed this directly but created an issue. Choosing an representation for Visibility is a thing that will affect us for a long time. This is something I would also like to discuss in the design repo. (I just wanted to create the PR first (which failed now -.-))

bors bot added a commit that referenced this issue Jun 30, 2022
27: Rustup -> nightly-2022-06-30 r=xFrednet a=xFrednet

This update is sadly not super clean, but I've crated an issue #26 for the introduced bug.

Co-authored-by: xFrednet <xFrednet@gmail.com>
@xFrednet
Copy link
Member Author

I'm creating an issue for this 🙃

@jhpratt
Copy link
Member

jhpratt commented Jun 30, 2022

Imo having structs with everything private is probably the simplest way to be forward compatible. Methods can exist to expose what's deemed necessary to construct and use values.

@xFrednet
Copy link
Member Author

I've created rust-marker/design#22

Imo having structs with everything private is probably the simplest way to be forward compatible. Methods can exist to expose what's deemed necessary to construct and use values.

I think that enums marked with non_exhaustive can be equivalently effective depending on the context. Using a struct for the visibility is something I haven't considered. Would you mind proposing that in the linked issue? 🙃

bors bot added a commit that referenced this issue Jun 30, 2022
27: Rustup -> nightly-2022-06-30 r=xFrednet a=xFrednet

This update is sadly not super clean, but I've crated an issue #26 for the introduced bug.

Co-authored-by: xFrednet <xFrednet@gmail.com>
@jhpratt
Copy link
Member

jhpratt commented Jun 30, 2022

I think that enums marked with non_exhaustive can be equivalently effective depending on the context.

Not necessarily, as the variants themselves are still always public. You might want to change the top-level structure, which would no longer be permissible. Leaving a comment there as requested momentarily.

@xFrednet
Copy link
Member Author

xFrednet commented Jun 30, 2022

Also true! I think in some instance it's unavoidable to use enums. When I've used them, I always wrap a single struct, which can be updated and expanded later. But that's not totally equivalent.

@xFrednet xFrednet added this to the v0.0.1 milestone Nov 10, 2022
@xFrednet xFrednet removed this from the v0.1.0 milestone Jul 16, 2023
Veetaha added a commit to Veetaha/marker that referenced this issue Oct 7, 2023
I added `HasSpan` trait to all types that had a custom `span()` method not included in that trait. I also replace `&Span` with `impl HasSpan` in all public methods in `marker_api` to allow passing nodes to them directly.

I even added a new lint to `marker_lints` that should catch the cases when `impl HasSpan` isn't used in a public method, but only after I implemented it I noticed that item visibility information isn't availble in `marker_api` yet. That's a bummer, because this custom lint doesn't make sense for non-pub methods and emits a lot of false positivies due to this fact. Anyway, since rust-marker#26 has to be eventually implemented I left the new custom lint in code, and added ignores at crate-level for it.
Veetaha added a commit to Veetaha/marker that referenced this issue Oct 7, 2023
I added `HasSpan` trait to all types that had a custom `span()` method not included in that trait. I also replace `&Span` with `impl HasSpan` in all public methods in `marker_api` to allow passing nodes to them directly.

I even added a new lint to `marker_lints` that should catch the cases when `impl HasSpan` isn't used in a public method, but only after I implemented it I noticed that item visibility information isn't availble in `marker_api` yet. That's a bummer, because this custom lint doesn't make sense for non-pub methods and emits a lot of false positivies due to this fact. Anyway, since rust-marker#26 has to be eventually implemented I left the new custom lint in code, and added ignores at crate-level for it.
Veetaha added a commit to Veetaha/marker that referenced this issue Oct 7, 2023
I added `HasSpan` trait to all types that had a custom `span()` method not included in that trait. I also replace `&Span` with `impl HasSpan` in all public methods in `marker_api` to allow passing nodes to them directly.

I even added a new lint to `marker_lints` that should catch the cases when `impl HasSpan` isn't used in a public method, but only after I implemented it I noticed that item visibility information isn't availble in `marker_api` yet. That's a bummer, because this custom lint doesn't make sense for non-pub methods and emits a lot of false positivies due to this fact. Anyway, since rust-marker#26 has to be eventually implemented I left the new custom lint in code, and added ignores at crate-level for it.
@xFrednet xFrednet added this to the v0.4.0 milestone Oct 7, 2023
Veetaha added a commit to Veetaha/marker that referenced this issue Oct 8, 2023
I added `HasSpan` trait to all types that had a custom `span()` method not included in that trait. I also replace `&Span` with `impl HasSpan` in all public methods in `marker_api` to allow passing nodes to them directly.

I even added a new lint to `marker_lints` that should catch the cases when `impl HasSpan` isn't used in a public method, but only after I implemented it I noticed that item visibility information isn't availble in `marker_api` yet. That's a bummer, because this custom lint doesn't make sense for non-pub methods and emits a lot of false positivies due to this fact. Anyway, since rust-marker#26 has to be eventually implemented I left the new custom lint in code, and added ignores at crate-level for it.
Veetaha added a commit to Veetaha/marker that referenced this issue Oct 8, 2023
I added `HasSpan` trait to all types that had a custom `span()` method not included in that trait. I also replace `&Span` with `impl HasSpan` in all public methods in `marker_api` to allow passing nodes to them directly.

I even added a new lint to `marker_lints` that should catch the cases when `impl HasSpan` isn't used in a public method, but only after I implemented it I noticed that item visibility information isn't availble in `marker_api` yet. That's a bummer, because this custom lint doesn't make sense for non-pub methods and emits a lot of false positivies due to this fact. Anyway, since rust-marker#26 has to be eventually implemented I left the new custom lint in code, and added ignores at crate-level for it.
@xFrednet xFrednet self-assigned this Oct 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-api Area: Stable API C-bug Category: Something isn't working D-rustc-driver Driver: Rustc Driver E-help-wanted Participation: Issues with some complexity and where help would be highly appreciated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants