Fix target_feature 1.1 should print the list of missing target features by rohaquinlop · Pull Request #109710 · rust-lang/rust

rohaquinlop · 2023-03-28T22:19:30Z

Fixed#108680

According to the issue description implemented the note and help description. Removed the note in the general case.

rustbot · 2023-03-28T22:19:38Z

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-03-28T22:19:41Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rohaquinlop · 2023-03-28T22:20:15Z

@est31 Already created the PR.

est31

This looks really great already, you will also need to bless the tests whose output has been changed by this PR. I offer you to help with this as you said you have M1 hardware which makes it hard to work on x86 specific tests: i can file a PR to your PR once it is ready in the other components.

bors · 2023-03-30T00:25:45Z

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

rohaquinlop · 2023-04-16T19:42:54Z

@estebank What should we do for the test outputs in another architectures?

est31 · 2023-04-16T19:57:17Z

LGTM except for the test issue 👍

…ures

rohaquinlop · 2023-06-07T19:23:19Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

est31 · 2023-06-08T00:46:46Z

 with interior mutability, the field can be changed to invalid values",
 ),
- CallToFunctionWith => (
+ CallToFunctionWith { missing_features: _, target_features: _ } => (


You can use { .. } here, this holds true for the other occurrences of this pattern in the last commit as well.

est31 · 2023-06-08T01:04:23Z

+ }
+ }
+
+ fn target_features(&self) -> Vec<Symbol> {


This should return Option<Vec<Symbol>> here instead of an empty vector. Then in the note_missing_features function, you don't need to check is_empty any more but can directly use self.target_features()?.

est31 · 2023-06-08T01:05:12Z

 }

 impl UnsafetyViolationDetails {
+ fn missing_features(&self) -> Vec<Symbol> {


Same here, missing_features should return an Option<Vec<Symbol>> instead.

est31 · 2023-06-08T01:10:48Z

+ diag.help(help);
+ }
+
+ match self.details.violation {


The outer match is not needed here, you can just collapse it to a single if let. If it's None, you just do what you do in the _ arm.

This was the old code

It's still part of the diff though? Maybe you haven't added these changes to git yet?

Ohh you meant what I am asking you to do reflects the older state of a PR, and you were asked to change it? I don't think so, but please correct me if I'm wrong. The older state also had a match, just no if let inside. Now you are doing both. Ideally you'd just have an if let, like:
ifletSome(note_missing_features) = self.details.violation.note_missing_features(){ diag.note(note_missing_features);}else{ diag.note(self.details.clone().note());}
The note_missing_features function will already return None if it's not a CallToFunctionWith, we just use this fact here.
My demand is more or less the same thing I asked about in march: #109710 (comment)
If you need more clarifications, please ask!

est31 · 2023-06-08T01:10:54Z

+ details: unsafe_details,
 op_in_unsafe_fn_allowed,
+ help: details.help_missing_features(),
+ note_missing_features: details.note_missing_features(),


You don't need to store this any more, you can just call note_missing_features() where you'd be using the note_missing_features field. Same goes for the help field.

est31 · 2023-06-08T01:12:10Z

+ if let Some(help) = self.help {
+ diag.help(help);
+ }
+ match self.details.violation {


Same holds here, this can be collapsed into a single if let Some(note_missing_features) = self.self.details.note_missing_features().

est31 · 2023-06-08T01:20:25Z

 diag.set_arg("details", desc);
- diag.span_label(self.details.span, self.details.label());
- diag.note(self.details.note());
+ diag.span_label(self.details.clone().span, self.details.clone().label());


I think you don't need to clone details to obtain the span, just to obtain the label.

It's necessary, if I don't clone it then will be an error here

bors · 2023-06-13T19:39:04Z

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

oli-obk · 2023-06-14T12:45:57Z

r? @est31 (feel free to bounce back to me if you prefer)

est31 · 2023-07-20T15:05:13Z

This has some still outstanding points and also needs a rebase, @rohaquinlop if you need advice for any of these please write!

rohaquinlop · 2023-07-21T02:50:11Z

@est31 I'll go back to work on this issue, thanks in advance

JohnCSimon · 2023-10-01T04:12:59Z

@rohaquinlop

ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Dylan-DPC · 2023-10-21T09:21:43Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

…tures, r=est31 Print list of missing target features when calling a function with target features outside an unsafe block Fixesrust-lang#108680 Supersedes rust-lang#109710. I used the same wording for the messages, but the implementation is different. r? `@est31`

…tures, r=est31 Print list of missing target features when calling a function with target features outside an unsafe block Fixesrust-lang#108680 Supersedes rust-lang#109710. I used the same wording for the messages, but the implementation is different. r? ``@est31``

…tures, r=est31 Print list of missing target features when calling a function with target features outside an unsafe block Fixesrust-lang#108680 Supersedes rust-lang#109710. I used the same wording for the messages, but the implementation is different. r? `@est31`

Rollup merge of rust-lang#118333 - eduardosm:print-missing-target-features, r=est31 Print list of missing target features when calling a function with target features outside an unsafe block Fixesrust-lang#108680 Supersedes rust-lang#109710. I used the same wording for the messages, but the implementation is different. r? `@est31`

rustbot assigned oli-obk Mar 28, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 28, 2023

This comment has been minimized.
Sign in to view

compiler-errors reviewed Mar 29, 2023
View reviewed changes

Comment threadtests/ui/target-feature/issue-108680-1.stderr Outdated

compiler-errors reviewed Mar 29, 2023
View reviewed changes

Comment threadcompiler/rustc_middle/src/mir/query.rs Outdated
Comment threadcompiler/rustc_middle/src/mir/query.rs Outdated

est31 requested changes Mar 29, 2023
View reviewed changes

Comment threadcompiler/rustc_middle/src/mir/query.rs Outdated
Comment threadcompiler/rustc_middle/src/mir/query.rs Outdated
Comment threadtests/ui/target-feature/issue-108680-1.rs Outdated

rohaquinlop force-pushed the 108680-target_feature-1.1 branch from d4ad06c to 3260f5cCompare March 30, 2023 20:46

This comment has been minimized.
Sign in to view

rohaquinlop requested review from compiler-errors, est31 and workingjubilee March 30, 2023 21:07

est31 requested changes Mar 30, 2023
View reviewed changes

rohaquinlop force-pushed the 108680-target_feature-1.1 branch from 3260f5c to 4270524Compare April 4, 2023 03:56

This comment has been minimized.
Sign in to view

est31 requested changes Apr 8, 2023
View reviewed changes

Comment threadcompiler/rustc_mir_transform/src/check_unsafety.rs Outdated
Comment threadcompiler/rustc_mir_transform/src/check_unsafety.rs Outdated

rohaquinlop force-pushed the 108680-target_feature-1.1 branch from 4270524 to d7a8af4Compare April 14, 2023 21:03

This comment has been minimized.
Sign in to view

rohaquinlop force-pushed the 108680-target_feature-1.1 branch 2 times, most recently from 50d411d to 63dc009Compare April 18, 2023 15:18

This comment has been minimized.
Sign in to view

rohaquinlop force-pushed the 108680-target_feature-1.1 branch from 63dc009 to fbc38d2Compare April 18, 2023 15:56

This comment has been minimized.
Sign in to view

rohaquinlop force-pushed the 108680-target_feature-1.1 branch from 802c173 to 004b5e0Compare June 6, 2023 23:29

This comment has been minimized.
Sign in to view

rohaquinlop force-pushed the 108680-target_feature-1.1 branch from 004b5e0 to ae93226Compare June 7, 2023 18:25

fixed target_feature 1.1 should print the list of missing target feat…
73fd02f
…ures

rohaquinlop force-pushed the 108680-target_feature-1.1 branch from ae93226 to 73fd02fCompare June 7, 2023 18:31

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 7, 2023

rohaquinlop requested a review from oli-obk June 7, 2023 19:44

est31 requested changes Jun 8, 2023
View reviewed changes

rustbot assigned est31 and unassigned oli-obk Jun 14, 2023

est31 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2023

est31 mentioned this pull request Sep 24, 2023
Tracking issue for target_feature 1.1 RFC #69098
Closed
10 tasks

Dylan-DPC closed this Oct 21, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2023

eduardosm mentioned this pull request Nov 26, 2023
Print list of missing target features when calling a function with target features outside an unsafe block #118333
Merged

Uh oh!

Uh oh!

Conversation

rohaquinlop commented Mar 28, 2023

Uh oh!

rustbot commented Mar 28, 2023

Uh oh!

rustbot commented Mar 28, 2023

Uh oh!

rohaquinlop commented Mar 28, 2023

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

est31 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Mar 30, 2023

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

This comment has been minimized.

rohaquinlop commented Apr 16, 2023

Uh oh!

est31 commented Apr 16, 2023

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rohaquinlop commented Jun 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

est31Jun 8, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jun 13, 2023

Uh oh!

oli-obk commented Jun 14, 2023

Uh oh!

est31 commented Jul 20, 2023

Uh oh!

rohaquinlop commented Jul 21, 2023

Uh oh!

JohnCSimon commented Oct 1, 2023

Uh oh!

Dylan-DPC commented Oct 21, 2023

Uh oh!

est31Jun 8, 2023•
edited
Loading