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

Avoid exposing type parameters and implementation details sourced from macro expansions #107789

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Feb 8, 2023

Fixes #107745.

I would like to request some guidance for this issue, because I don't think this is a good fix (a band-aid at best).

The Problem

The code

fn main() {
    println!("{:?}", []);
}

gets desugared into (rustc +nightly --edition=2018 issue-107745.rs -Z unpretty=hir):

#[prelude_import]
use std::prelude::rust_2018::*;
#[macro_use]
extern crate std;
fn main() {
        {
                ::std::io::_print(<#[lang = "format_arguments"]>::new_v1(&["",
                                    "\n"], &[<#[lang = "format_argument"]>::new_debug(&[])]));
            };
    }

so the diagnostics code tries to be as specific and helpful as possible, and I think it finds that [] needs a type parameter and so does new_debug. But since [] doesn't have an origin for the type parameter definition, it points to new_debug instead and leaks the internal implementation detail since all [] has is an type inference variable.

The Bad Fix

This PR currently tries to fix the problem by bypassing the generated function <#[lang = "format_argument"]>::new_debug to avoid its generic parameter (I think it is auto-generated from the argument [_; 0]?) from getting collected as an InsertableGenericArg. This is problematic because it also prevents the help from getting displayed.

I think this fix is not ideal and hard-codes the format generated code pattern, but I can't think of a better fix. I have tried asking on Zulip but no responses there yet.

@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2023

r? @jackh726

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

@rustbot 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 Feb 8, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 8, 2023

Some changes occurred in need_type_info.rs

cc @lcnr

@lcnr
Copy link
Contributor

lcnr commented Feb 8, 2023

I think the more general fix is to not consider any expressions which are from a macro expansion in the diagnostic here

I think the best fix is to change the way we avoid stuff from macro expansions. We currently simply cause the affected source to have a very high cost:

let suggestion_may_apply = if source.from_expansion() { 10000 } else { 0 };

It should be easier to instead completely ignore such sources. This may need a bit of experimentation

@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 8, 2023

I think tests/ui/issues/issue-16966.stderr needs to be updated because it also leaks implementation detail on panic!().

Currently experimenting to figure out how to fully block macro expansion sources, because while simply skip updating infer source if it's from a macro expansion works for the format!() case, it doesn't seem to work for

error[E0282]: type annotations needed
 --> tests/ui/issues/issue-16966.rs:2:12
  |
2 |     panic!(std::default::Default::default());
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type for type parameter `M` declared on the function `begin_panic`

which before skipping infer sources from macro expansions was

error[E0282]: type annotations needed
  --> $DIR/issue-16966.rs:2:5
   |
LL |     panic!(std::default::Default::default());
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `M` declared on the function `begin_panic`
   |
   = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)

@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 8, 2023

I found out how to cover the panic!() case, but plugging the panic!() leak causes three tests to fail...

failures:
    [ui] tests/ui/parser/missing-closing-angle-bracket-eq-constraint.rs
    [ui] tests/ui/type/type-check/cannot_infer_local_or_vec.rs
    [ui] tests/ui/type/type-check/cannot_infer_local_or_vec_in_tuples.rs

Since apparently if I got rid of the T this suggestion only has a placeholder _ left...

error[E0282]: type annotations needed for `Vec<T>`
 --> tests/ui/type/type-check/cannot_infer_local_or_vec.rs:2:9
  |
2 |     let x = vec![];
  |         ^
-Ztrack-diagnostics: created at compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs:559:14
  |
help: consider giving `x` an explicit type, where the placeholders `_` are specified
  |
2 |     let x: Vec<T> = vec![];
  |          ++++++++

whereas previously it was

error[E0282]: type annotations needed for `Vec<T>`
  --> $DIR/cannot_infer_local_or_vec.rs:2:9
   |
LL |     let x = vec![];
   |         ^
   |
help: consider giving `x` an explicit type, where the type for type parameter `T` is specified
   |
LL |     let x: Vec<T> = vec![];
   |          ++++++++

error: aborting due to previous error

@lcnr
Copy link
Contributor

lcnr commented Feb 8, 2023

hmm, idk 😅 that requires some fiddling around, maybe only use the parameter name and don't explain where it comes from? I think it should be enough to simply not add a parent to the InferenceDiagnosticsData 🤷

@lcnr lcnr closed this Feb 8, 2023
@lcnr lcnr reopened this Feb 8, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 9, 2023

hmm, idk 😅 that requires some fiddling around, maybe only use the parameter name and don't explain where it comes from? I think it should be enough to simply not add a parent to the InferenceDiagnosticsData 🤷

I think I fixed the vec![] cases. Updated fmt_printer to not print type parameters whose source is from macro expansions.

@jieyouxu jieyouxu changed the title Avoid leaking format!() desugaring when empty array arguments are supplied Avoid exposing type parameters and implementation details sourced from macro expansions Feb 9, 2023
@lcnr
Copy link
Contributor

lcnr commented Feb 10, 2023

r? @lcnr

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 10, 2023

📌 Commit b58347a has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#107789 (Avoid exposing type parameters and implementation details sourced from macro expansions)
 - rust-lang#107836 (Handle properly when there is no crate attrs)
 - rust-lang#107839 (avoid duplicating the RUSTC_LOG env var name)
 - rust-lang#107866 (Allow wasi-libc to initialize its environment variables lazily.)
 - rust-lang#107876 (create symlink only for non-windows operating systems)
 - rust-lang#107882 (Cleanup typos in en_US/borrowck.ftl)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dc7559b into rust-lang:master Feb 10, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 10, 2023
@jieyouxu jieyouxu deleted the issue-107745 branch June 15, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatting empty array leaks internal function name new_debug
6 participants