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

Sync from downstream #14980

Merged
merged 17 commits into from Jun 5, 2023
Merged

Sync from downstream #14980

merged 17 commits into from Jun 5, 2023

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jun 5, 2023

No description provided.

michaelvanstraten and others added 17 commits March 10, 2023 22:16
Added byte position range for `proc_macro::Span`

Currently, the [`Debug`](https://doc.rust-lang.org/beta/proc_macro/struct.Span.html#impl-Debug-for-Span) implementation for [`proc_macro::Span`](https://doc.rust-lang.org/beta/proc_macro/struct.Span.html#) calls the debug function implemented in the trait implementation of `server::Span` for the type `Rustc` in the `rustc-expand` crate.

The current implementation, of the referenced function, looks something like this:
```rust
fn debug(&mut self, span: Self::Span) -> String {
    if self.ecx.ecfg.span_debug {
        format!("{:?}", span)
    } else {
        format!("{:?} bytes({}..{})", span.ctxt(), span.lo().0, span.hi().0)
    }
}
```

It returns the byte position of the [`Span`](https://doc.rust-lang.org/beta/proc_macro/struct.Span.html#) as an interpolated string.

Because this is currently the only way to get a spans position in the file, I might lead someone, who is interested in this information, to parsing this interpolated string back into a range of bytes, which I think is a very non-rusty way.

The proposed `position()`, method implemented in this PR, gives the ability to directly get this info.
It returns a [`std::ops::Range`](https://doc.rust-lang.org/std/ops/struct.Range.html#) wrapping the lowest and highest byte of the [`Span`](https://doc.rust-lang.org/beta/proc_macro/struct.Span.html#).

I put it behind the `proc_macro_span` feature flag because many of the other functions that have a similar footprint also are annotated with it, I don't actually know if this is right.

It would be great if somebody could take a look at this, thank you very much in advanced.
Report allocation errors as panics

OOM is now reported as a panic but with a custom payload type (`AllocErrorPanicPayload`) which holds the layout that was passed to `handle_alloc_error`.

This should be review one commit at a time:
- The first commit adds `AllocErrorPanicPayload` and changes allocation errors to always be reported as panics.
- The second commit removes `#[alloc_error_handler]` and the `alloc_error_hook` API.

ACP: rust-lang/libs-team#192

Closes #51540
Closes #51245
This reverts commit abc0660118cc95f47445fd33502a11dd448f5968.
More core::fmt::rt cleanup.

- Removes the `V1` suffix from the `Argument` and `Flag` types.

- Moves more of the format_args lang items into the `core::fmt::rt` module. (The only remaining lang item in `core::fmt` is `Arguments` itself, which is a public type.)

Part of rust-lang/rust#99012

Follow-up to rust-lang/rust#110616
This function/lang_item was introduced in #104321 as a temporary workaround of future lowering.
The usage and need for it went away in #104833.
After a bootstrap update, the function itself can be removed from `std`.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2023
@lnicola
Copy link
Member Author

lnicola commented Jun 5, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 5, 2023

📌 Commit c3dbe7c has been approved by lnicola

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 5, 2023

⌛ Testing commit c3dbe7c with merge aa9bc86...

@bors
Copy link
Collaborator

bors commented Jun 5, 2023

☀️ Test successful - checks-actions
Approved by: lnicola
Pushing aa9bc86 to master...

@bors bors merged commit aa9bc86 into rust-lang:master Jun 5, 2023
10 checks passed
@lnicola lnicola deleted the sync-from-rust branch June 5, 2023 08:51
@Veykril
Copy link
Member

Veykril commented Jun 6, 2023

Fwiw, this brought the mir bodies back to fail @HKalbasi, i imagine thats because of the fmt argument changes which match latest nightly, but not current stable. Kind of fun how r-a actually breaks because of implementation detail changes 🙂 Are those types lang items? If yes we could maybe somehow make this emit lang items to get around that.

@lnicola
Copy link
Member Author

lnicola commented Jun 6, 2023

Even before, our expansion was slightly different from that of rustc, but I didn't really understand why 😔.

@Veykril
Copy link
Member

Veykril commented Jun 6, 2023

The previous impl was mainly done to satisfy type inference for macro expression as a whole I think

@Veykril
Copy link
Member

Veykril commented Jun 6, 2023

That remidns me I played around with the concept of lang item paths, wonder if I still have that stash/branch

@HKalbasi
Copy link
Member

Ah, I missed the ping, and saw the ping now that I come to this to see what breaks the mir. Yes, it's the Argument change, and it isn't just the mir, the unknown types are also significantly increased, and I guess it may affect some other ide features in a bad way.

I don't see how it is possible to emit lang item paths in macro expanding, but if it is possible to make it working, it is lang item (at least it was in the old version) so I guess there is no problem from this side.

But there is a more general problem here, syncing builtin proc-macro implementations with the rustc, which the lang item path solution is not always available for it. An idealistic solution is to share the implementation with the installed rustc somehow, or make them simple proc-macros. A realistic approach is to conditionally expand builtin macros based on the rustc target version, or just reverting this and re applying it when it hits stable. #14846 is maybe also related.

@Veykril
Copy link
Member

Veykril commented Jun 16, 2023

I don't see how it is possible to emit lang item paths in macro expanding,

We would basically allow some special syntax that we'd interpret as a lang item path, something like lang # lang_item_name

A realistic approach is to conditionally expand builtin macros based on the rustc target version

That's not a realistic approach, then we are back to having to maintain multiple versions of things which we don't want. The way I see it, we either need to ask upstream to make anything that's implementation detail emitted by builtin macros lang items so the names changing doesn't matter or have them be turned into proper proc-macros whose dylibs are shipped with the toolchain.

@HKalbasi
Copy link
Member

We would basically allow some special syntax that we'd interpret as a lang item path, something like lang # lang_item_name

Another problem here is that Argument is a lang item, but Argument::new is what we want so it wouldn't work with current lang paths.

That's not a realistic approach, then we are back to having to maintain multiple versions of things which we don't want.

We could drop support when it passed some time, and keep only e.g. two latest stable. But okay, let's focus on the lang paths or just drop it until it hits stable.

we either need to ask upstream to make anything that's implementation detail emitted by builtin macros lang items so the names changing doesn't matter

What I understand from rust-lang/rust#99012 is that it probably wouldn't end in naming changes only. What we can ask is to keeping the old version working for some time.

or have them be turned into proper proc-macros whose dylibs are shipped with the toolchain.

I like this, but I don't think it would work with format_args! as it is too special. It can probably work for other builtin macros, and the less builtin macro, the better. But it would also probably makes some headaches (for one time, hopefully) for us as we don't support std lib dependencies (by the way, is there any news on that issue?)

@Veykril
Copy link
Member

Veykril commented Jun 16, 2023

Another problem here is that Argument is a lang item, but Argument::new is what we want so it wouldn't work with current lang paths.

It could if we allowed something like lang#foo::new (that is the lang#lang_name is treated as a path segment)

I like this, but I don't think it would work with format_args! as it is too special.

Right, format_args is an eager macro so that wouldn't work as a proc-macro yet... (but soon, there is a nightly feature for that)

@HKalbasi
Copy link
Member

Right, format_args is an eager macro so that wouldn't work as a proc-macro yet... (but soon, there is a nightly feature for that)

The format_args!("{a}") also couldn't be implemented in proc macros due hygienic I think.

@Veykril
Copy link
Member

Veykril commented Jun 16, 2023

(by the way, is there any news on that issue?)

forgot to reply to this, no news there

@HKalbasi
Copy link
Member

The format_args is broken even on nightly. The problem is that Argument is defined inside core::fmt::rt which is a private module so even if we use core::fmt::rt::Argument (we are currently using core::fmt::Argument) it wouldn't work. So it seems lang paths are the only way to access that. We need to either make the lang paths a special ast node, or make the entire format_args!() a special ast node similar to what rustc does.

@Veykril
Copy link
Member

Veykril commented Jun 19, 2023

or make the entire format_args!() a special ast node similar to what rustc does.

This I never understood how they do since you need to resolve the path first before knowing if its a format_args invocation, but resolution obviously happens after AST building.

@HKalbasi
Copy link
Member

I think format_args!() is still a macro, it just will be expanded to the special ast node.

@Veykril Veykril mentioned this pull request Jun 19, 2023
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.

None yet