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

Support multiple targets for checkOnSave (in conjunction with cargo 1.64.0+) #13290

Merged
merged 2 commits into from
Nov 19, 2022

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Sep 24, 2022

This PR adds support for the ability to pass multiple --target flags when using
cargo 1.64.0+.

Questions

I needed to change the type of two configurations options, but I did not plurialize the names to
avoid too much churn, should I ?

Zulip thread

https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/Issue.2013282.20.28supporting.20multiple.20targets.20with.201.2E64.2B.29

Example

To see it working, on a macOS machine:

$ cd /tmp
$ cargo new cargo-multiple-targets-support-ra-test
$ cd !$
$ mkdir .cargo
$ echo '
[build]
target = [
    "aarch64-apple-darwin",
    "x86_64-apple-darwin",
]
' > .cargo/config.toml
$ echo '
fn main() {
    #[cfg(all(target_arch = "aarch64", target_os = "macos"))]
    {
        let a = std::fs::read_to_string("/tmp/test-read");
    }

    #[cfg(all(target_arch = "x86_64", target_os = "macos"))]
    {
        let a = std::fs::read_to_string("/tmp/test-read");
    }

    #[cfg(all(target_arch = "x86_64", target_os = "windows"))]
    {
        let a = std::fs::read_to_string("/tmp/test-read");
    }
}
' > src/main.rs
# launch your favorite editor with the version of RA from this PR
#
# You should see warnings under the first two `let a = ...` but not the third

Screen

Two panes of a terminal emulator, on the left pane is the main.rs file described above, with warnings for the first two let a = declaration, on the right pane is a display of the .cargo/config.toml, an ls of the current files in the directory and a call to cargo build to show the same warnings as in the editor on the left pane

Helps with #13282

@poliorcetics poliorcetics force-pushed the multiple-targets branch 2 times, most recently from 65e8b97 to abb3f0e Compare September 25, 2022 09:30
@poliorcetics
Copy link
Contributor Author

I didn't make the change for the rust-project.json config because rustc does not accept multiple --target flags, contrarily to cargo

@flodiebold
Copy link
Member

I don't think we can support this for cargo.target, at most for checkOnSave.

@poliorcetics
Copy link
Contributor Author

I don't think we can support this for cargo.target, at most for checkOnSave.

cargo on 1.64.0+ accepts multiple targets, it's even supported in the .cargo/config.toml, why would cargo.target not support it ?

@flodiebold
Copy link
Member

Because cargo.target controls our analysis, and that doesn't support multiple targets (without a lot more code).

@poliorcetics
Copy link
Contributor Author

Because cargo.target controls our analysis, and that doesn't support multiple targets (without a lot more code).

Oh, I'll try to do it, either in this PR if it's not merged by then, or in another. For now, I removed it from the commit and changed back to an Option.

@poliorcetics
Copy link
Contributor Author

Progress report

I made some progress on this today 🎉.

I have completions working for several targets at once:

std::arch::x86_64::__m12 correctly proposes the completion for __m128

But (because of course it's not perfectly working), completions for conflicting names fail, I don't which is chosen first (I suspect the first configured target):

sys_func(usize, usize) is selected instead of the single-argument variant, which is the one to use on x86_64-apple-darwin

@poliorcetics poliorcetics force-pushed the multiple-targets branch 2 times, most recently from 16c50a4 to 54efd42 Compare October 9, 2022 20:26
@poliorcetics
Copy link
Contributor Author

Go to definition and errors will point to the wrong item with the implementation I used, I removed that part and kept only the multiple targets when running cargo, not inside R-A

@bors
Copy link
Contributor

bors commented Oct 19, 2022

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

@Veykril Veykril added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2022
@poliorcetics poliorcetics force-pushed the multiple-targets branch 2 times, most recently from 39aa7b0 to 12f5d80 Compare November 9, 2022 11:28
@Veykril Veykril changed the title Support multiple targets (in conjunction with cargo 1.64.0+) Support multiple targets for checkOnSave (in conjunction with cargo 1.64.0+) Nov 19, 2022
@Veykril
Copy link
Member

Veykril commented Nov 19, 2022

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Nov 19, 2022

📌 Commit 0d4737a has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 19, 2022

⌛ Testing commit 0d4737a with merge 38fa47f...

@bors
Copy link
Contributor

bors commented Nov 19, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 38fa47f to master...

@bors bors merged commit 38fa47f into rust-lang:master Nov 19, 2022
@poliorcetics poliorcetics deleted the multiple-targets branch November 19, 2022 13:36
iredelmeier added a commit to iredelmeier/rust-analyzer that referenced this pull request Nov 21, 2022
This fixes a regression introduced by rust-lang#13290, in which failing to set
`checkOnSave/target` (or `checkOnSave/targets`) would lead to an invalid
config.
iredelmeier added a commit to iredelmeier/rust-analyzer that referenced this pull request Nov 21, 2022
This fixes a regression introduced by rust-lang#13290, in which failing to set
`checkOnSave/target` (or `checkOnSave/targets`) would lead to an invalid
config.
bors added a commit that referenced this pull request Nov 24, 2022
…as-schievink

Fix: Handle empty `checkOnSave/target` values

This fixes a regression introduced by #13290, in which failing to set `checkOnSave/target` (or `checkOnSave/targets`) would lead to an invalid config.

[Fixes #13660]
arcnmx added a commit to arcnmx/rust that referenced this pull request Dec 17, 2022
The prior update included checkOnSave multiple targets:
rust-lang/rust-analyzer#13290
but missed the fix for the regression it caused:
rust-lang/rust-analyzer#13661

Merge commit '6d61be8e65ac0fd45eaf178e1f7a1ec6b582de1f'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants