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

cargo-marker refactoring and review #66

Merged
merged 6 commits into from
Jan 5, 2023
Merged

cargo-marker refactoring and review #66

merged 6 commits into from
Jan 5, 2023

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Dec 1, 2022

I've refactored parts of cargo-marker and reviewed the implementation. I found several limitations which are now documented in #60. This PR already fixes a few of these.

CC: #60

Same as always: If I don't get a review, I'll merge this, when a depending PR is ready. :)

@xFrednet xFrednet added the S-waiting-on-review Status: Awaiting review label Dec 1, 2022
@xFrednet

This comment was marked as off-topic.

Sign in to view
bors bot added a commit that referenced this pull request Dec 1, 2022
@bors
Try #66:
1812666
@bors

This comment was marked as outdated.

Sign in to view
@xFrednet xFrednet force-pushed the 060-cargo-cli-rework-and-pain branch from 1db411c to bc7211c Compare December 1, 2022 11:18
@xFrednet

This comment was marked as outdated.

Sign in to view
bors bot added a commit that referenced this pull request Dec 1, 2022
@bors
Try #66:
7c6d6dc
@bors

This comment was marked as outdated.

Sign in to view
@xFrednet xFrednet mentioned this pull request Dec 1, 2022
Closed
@xFrednet xFrednet force-pushed the 060-cargo-cli-rework-and-pain branch from bc7211c to 3202642 Compare December 1, 2022 22:53
@xFrednet

This comment was marked as off-topic.

Sign in to view
bors bot added a commit that referenced this pull request Dec 1, 2022
@bors
Try #66:
3a9e0dc
@bors

This comment was marked as off-topic.

Sign in to view
@xFrednet xFrednet force-pushed the 060-cargo-cli-rework-and-pain branch from 3202642 to 16e8d4a Compare December 1, 2022 22:58
@xFrednet

This comment was marked as off-topic.

Sign in to view
bors bot added a commit that referenced this pull request Dec 1, 2022
@bors
Try #66:
6d7b566
@bors

This comment was marked as outdated.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
@xFrednet xFrednet force-pushed the 060-cargo-cli-rework-and-pain branch from 16e8d4a to 60e1fa4 Compare December 1, 2022 23:05
@xFrednet

This comment was marked as off-topic.

Sign in to view
bors bot added a commit that referenced this pull request Dec 1, 2022
@bors
Try #66:
75a0dd6
@bors

This comment was marked as outdated.

Sign in to view
@xFrednet

This comment was marked as outdated.

Sign in to view
bors bot added a commit that referenced this pull request Dec 1, 2022
@bors
Try #66:
43fb71e
@bors

This comment was marked as outdated.

Sign in to view
@xFrednet xFrednet force-pushed the 060-cargo-cli-rework-and-pain branch from 8f5106b to 813df56 Compare December 1, 2022 23:24
@xFrednet
Copy link
Member Author

xFrednet commented Dec 1, 2022

bors try

bors bot added a commit that referenced this pull request Dec 1, 2022
@bors
Try #66:
1399dbc
@bors
Copy link
Contributor

bors bot commented Dec 1, 2022

try

Build failed:

@xFrednet xFrednet force-pushed the 060-cargo-cli-rework-and-pain branch from 813df56 to c26b868 Compare December 1, 2022 23:31
@xFrednet
Copy link
Member Author

xFrednet commented Dec 1, 2022

bors try

bors bot added a commit that referenced this pull request Dec 1, 2022
@bors
Try #66:
3f6ee25
@bors
Copy link
Contributor

bors bot commented Dec 1, 2022

try

Build succeeded:

@xFrednet xFrednet added the A-marker-cargo Area: All things connected to `cargo_marker` label Dec 7, 2022
@xFrednet xFrednet added this to the v0.0.1 milestone Dec 7, 2022
Niki4tap
Niki4tap reviewed Dec 31, 2022
View reviewed changes
cargo-marker/src/cli.rs
.args(check_command_args())
}

fn check_command_args() -> impl IntoIterator<Item = impl Into<Arg>> {
Copy link
Member

Choose a reason for hiding this comment

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

Hey, not to pile up on an already huge pile of work you're doing, but this impl return type, causes marker_driver_rustc to panic, when trying to lint with default test lint in marker_lints.

Seems like it's because of this TODO in here, just saying that you might want to get that fixed before merging this :)

Panic 
thread '<unnamed>' panicked at 'not yet implemented: Ty {
    hir_id: HirId {
        owner: OwnerId {
            def_id: DefId(0:24 ~ cargo_marker[a207]::cli::check_command_args),
        },
        local_id: 37,
    },
    kind: OpaqueDef(
        ItemId {
            owner_id: OwnerId {
                def_id: DefId(0:165 ~ cargo_marker[a207]::cli::check_command_args::{opaque#0}),
            },
        },
        [],
        false,
    ),
    span: cargo-marker/src/cli.rs:54:28: 54:68 (#0),
}', marker_driver_rustc/src/conversion/ty.rs:88:17

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @Niki4tap, nice to see you in this repo. Welcome to marker!

not to pile up on an already huge pile of work you're doing

No worries at all. I appreciate reviews, it might take a bit more work, but that's better than fixing the code later again. It's also helpful to discuss alternative solutions.

The impl Trait type slipped my mind when I worked on the rustc backend. #55 includes a change in rustc that effects how impl Trait is represented. I waited for the nightly version until removing the Todo. Then life happened, and it's already the end of December ^^.

I might merge this PR before the nightly bump to avoid conflicts, but then I'll try to fix it in that PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Welcome to marker!

Thanks!

I waited for the nightly version until removing the Todo. Then life happened, and it's already the end of December ^^.

Yeah, that tends to happen

I might merge this PR before the nightly bump to avoid conflicts, but then I'll try to fix it in that PR :)

No problem, just making sure you're aware of the issue :)

@xFrednet
Copy link
Member Author

xFrednet commented Jan 5, 2023

I'll merge this now, to allow further progress on cargo-marker the ICE is not yet fixed, but I'll do it as part of the nightly bump. That should be done tomorrow :)

@xFrednet
Copy link
Member Author

xFrednet commented Jan 5, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 5, 2023

Build succeeded:

@bors bors bot merged commit 0a449ed into rust-marker:master Jan 5, 2023
@xFrednet xFrednet mentioned this pull request Jan 5, 2023
Closed
@xFrednet xFrednet deleted the 060-cargo-cli-rework-and-pain branch January 6, 2023 00:06
@xFrednet xFrednet mentioned this pull request Jan 6, 2023
Merged
bors bot added a commit that referenced this pull request Jan 6, 2023
@bors @xFrednet
Merge #80
cc79eaf 
80: Fix ICE for `-> impl Trait` r=Niki4tap a=xFrednet

This fixes an ICE for `-> impl Trait` representations. It doesn't work with `-> impl Trait<T>` yet, where `T` is a generic of the item.

I'm not a 100% happy with how the rustc backend is structured, as it feels messy and hard to follow. I'm sadly not sure how to structure it better, as I've never worked on a translation layer like this in rust. My guess is that these things are always a bit messy, but I still hope to find a cleaner solution. For now, I hope that this is good enough.

I also found a small bug with the refactoring from #66 that `cargo dogfood` didn't recompile the driver. It should now do that again :)

r? `@Niki4tap` 

Co-authored-by: xFrednet <xFrednet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Niki4tap Niki4tap Niki4tap left review comments

Assignees
No one assigned
Labels
A-marker-cargo Area: All things connected to `cargo_marker` S-waiting-on-review Status: Awaiting review
Projects
None yet
Milestone
v0.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@xFrednet @Niki4tap