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

Unknown types in web-sys dependency #4150

Closed
eminence opened this issue Apr 26, 2020 · 31 comments
Closed

Unknown types in web-sys dependency #4150

eminence opened this issue Apr 26, 2020 · 31 comments
Labels
A-macro macro expansion

Comments

@eminence
Copy link
Contributor

eminence commented Apr 26, 2020

(I suspect this is a duplicate, but i'm not sure what the root cause is).

I'm trying to use the web-sys crate, which is highly generated.

I've enabled both loadOutDirsFromCheck and procMacro.enable:

.vscode/settings.jon
{
    "rust-analyzer.cargo.loadOutDirsFromCheck": true,
    "rust-analyzer.procMacro.enable": true,
    "editor.tokenColorCustomizationsExperimental": {
        "unresolvedReference": "#FF0000"
    }
}
Cargo.toml
[package]
name = "foo"
version = "0.1.0"
authors = ["Andrew Chin <[email protected]>"]
edition = "2018"

[dependencies.web-sys]
version = "0.3"
features = ["Document", "Window"]

Code:

use web_sys::{Document, Window};

pub fn foo() {
    let win: Window = web_sys::window().unwrap();
    let doc: Document = win.document().unwrap();
}

image

Using RA nightly (fe99a29)

Thanks!

@flodiebold
Copy link
Member

Are you using the weekly build? All these definitions are in extern "C" blocks, which we ignored until a few days ago. Apart from that, proc macro support is of course still very experimental and web_sys makes very heavy use of proc macros.

@eminence
Copy link
Contributor Author

I am using the nightly build (fe99a29). (Sorry, that was an important detail I should not have omitted)

@edwin0cheng
Copy link
Member

edwin0cheng commented Apr 27, 2020

For example, Here is how web-sys implement Window type. It seem like there is something we are still not yet implemented in RA :

  1. proc-macro for custom-attribues (#[wasm_bindgen]), we only support custom-derive now.
  2. Extern Type : extern "C" { pub type Window; }

@edwin0cheng edwin0cheng added the A-macro macro expansion label Apr 27, 2020
@matklad
Copy link
Member

matklad commented Apr 27, 2020

extern "C should be handled by #4111. Though, not sure about extern "C" { pub type Window; }. Opaque types are unstable?

@flodiebold
Copy link
Member

It probably gets turned into something different by the macro?

@edwin0cheng
Copy link
Member

rust-lang/rust#44295

And we didn't expand it because no custom attr proc-macro support yet. But I doubt if extern types are supported in RA, it might be infered properly ?

@danbrodsky
Copy link

Not to hijack but I'm having similar issues with both go-to-definition and completion for the web-sys crate. doing web_sys::ge[completion here] correctly shows completions for web-sys functions but doing something like let canvas = <some web-sys object>; canvas.ge[completion here] does not. I'm using emacs with lsp and have the following set:

        lsp-rust-analyzer-cargo-load-out-dirs-from-check t
        lsp-rust-analyzer-proc-macro-enable t

@crlf0710
Copy link
Member

crlf0710 commented Sep 5, 2020

Extern opaque types are supported by chalk now. Hopefully this will help with this issue.

@eminence
Copy link
Contributor Author

eminence commented Sep 9, 2020

Cool, that helps a bit!

Using the latest commit (60c8941), the Document and Window types are now resolved, though RA still doesn't know about the methods on them:

image

Edit: Actually, the upgraded chalk isn't in RA head yet, so the above improvements came from elsewhere... Sorry for the noise!

@crlf0710
Copy link
Member

I gave adding support for extern opaque types a try in #5993 .

@crlf0710
Copy link
Member

That PR has landed. But that didn't help web-sys much. If web-sys was using genuine extern opaque type syntax as input to macros (e.g. impl blocks etc) the issue would have been solved. It seems we need to improve proc macro support next. I'll look into what i can do to help.

@jkelleyrtp
Copy link
Contributor

jkelleyrtp commented Nov 21, 2020

Is there any movement on this front, or direction contributors could look to have in trying to get this supported? For reference, this the definition of web-sys modules.

#[wasm_bindgen]
extern "C" {
    # [wasm_bindgen (extends = UiEvent , extends = Event , extends = :: js_sys :: Object , js_name = InputEvent , typescript_type = "InputEvent")]
    #[derive(Debug, Clone, PartialEq, Eq)]
    pub type InputEvent;

    # [wasm_bindgen (structural , method , getter , js_class = "InputEvent" , js_name = isComposing)]
    pub fn is_composing(this: &InputEvent) -> bool;

    #[wasm_bindgen(catch, constructor, js_class = "InputEvent")]
    pub fn new(type_: &str) -> Result<InputEvent, JsValue>;

    #[cfg(feature = "InputEventInit")]
    #[wasm_bindgen(catch, constructor, js_class = "InputEvent")]
    pub fn new_with_event_init_dict(
        type_: &str,
        event_init_dict: &InputEventInit,
    ) -> Result<InputEvent, JsValue>;
}

@lnicola
Copy link
Member

lnicola commented Jan 22, 2021

Attribute proc macros are tracked in #6029, let's close this for now.

@lnicola lnicola closed this as completed Jan 22, 2021
@matklad
Copy link
Member

matklad commented Sep 2, 2021

@jonas-schievink do we expand attributes for exten blocks?

#[wasm_bindgen]
extern "C" {
    pub type Window;
}

https://users.rust-lang.org/t/no-autocompletion-with-rust-analyzer-and-wasm-bindgen-web-sys/64301

@jonas-schievink
Copy link
Contributor

We should

@flodiebold
Copy link
Member

If it's not working even with attribute macro support, we should reopen?

@jonas-schievink
Copy link
Contributor

Ah, the problem is that turning rust-analyzer.experimental.procAttrMacros on will make you run into #8158, and turning it off will also turn it off within web-sys, which means that the API doesn't get generated.

@matklad
Copy link
Member

matklad commented Sep 2, 2021

damned if you do, damned if you don't... I am wondering... Can we just record two functions with the same name in the def map, one expanded and one not expanded, such that source2def returns non-expanded version, but everything which tries to resolve paths gives you the resolved one?

@matklad
Copy link
Member

matklad commented Sep 2, 2021

image

Something like this I mean.

@flodiebold
Copy link
Member

flodiebold commented Sep 2, 2021

So document.x<|> should work, right?

And the proper fix might be for the wasm_bindgen macro to be more error resilient? Not sure we can do anything to help the proc macros there, right?

@flodiebold
Copy link
Member

damned if you do, damned if you don't... I am wondering... Can we just record two functions with the same name in the def map, one expanded and one not expanded, such that source2def returns non-expanded version, but everything which tries to resolve paths gives you the resolved one?

That seems like it would lead to all kinds of other incorrect results?

@matklad
Copy link
Member

matklad commented Sep 2, 2021

So document.x<|> should work, right?

Right. It will do completion in the non-expanded code. That's technically wrong (for example, macro can be written in such a way as to inject unhygienic document variable into functions local scope), but should be good enough most of the time. But the name resolution context outside of the function will be from expanded code, so, Window will be of the right type, etc.

And the proper fix might be for the wasm_bindgen macro to be more error resilient?

I don't think a fully proper fix is possible, as the general case is compile time reflection.

I think 90% proper fix would be to allow proc macro authors to annotate their macros as "this doesn't change item semantics", so that we know that the treatment, suggested above, is fully correct.

For the cases where the macro does change semantics, yeah, I think will need extra collaboration from the macro itself.

@jonas-schievink
Copy link
Contributor

I lied, it's not just #8158, but also #9866 that make completion not work here

@flodiebold
Copy link
Member

I don't think a fully proper fix is possible, as the general case is compile time reflection.

I think a fully proper fix is possible as long as the cursor is in a range of tokens that just get mapped as one continuous range, which I think is what people expect. No-one would reasonably expect completions in mirror! ;) IMO we should be able to handle attribute macros the same way we handle macro_rules, i.e. #9866.

@flodiebold
Copy link
Member

I think 90% proper fix would be to allow proc macro authors to annotate their macros as "this doesn't change item semantics", so that we know that the treatment, suggested above, is fully correct.

I'm not sure we can do this in a useful way? For example, it seems wasm_bindgen(start) on a function doesn't change the function, but wasm_bindgen on other items has noticeable effects. We could maybe notice that the macro didn't actually change the function though?

@flodiebold
Copy link
Member

flodiebold commented Sep 2, 2021

... although I guess the problem is that wasm_bindgen(start) doesn't actually pass through the function when there's a syntax error in it 🤔

Edit: Or does it? I don't know, maybe it will be fixed by just #9866...

@jonas-schievink
Copy link
Contributor

Yeah, proc macros generally just expand to compile_error!("syntax error at xyz"); when there's a syntax error in their input (due to how syn works).

An optimal fix for this would entail rewriting syn to be error-resilient and migrating the proc macro ecosystem over to that version. Quite a bit of work.

@jonas-schievink
Copy link
Contributor

Maybe we can just lie to the macro by synthesizing tokens that make the input parse correctly 🤔

@flodiebold
Copy link
Member

flodiebold commented Sep 2, 2021

The reason I was wondering is that in this case, the proc macro doesn't need to inspect the body of the function, so it would be fine for it to just parse the header, also for build time and forward compatibility reasons. I don't know if it does that right now though.

Maybe we can just lie to the macro by synthesizing tokens that make the input parse correctly thinking

Yeah, that's also something I was considering, though it could also lead to other problems. (It's also kind of similar to the hypothetically-expand-with-inserted-token thing we're doing, and maybe if we someday have better support for hypothetical queries in Salsa that could help as well.)

@flodiebold
Copy link
Member

I think the issues discussed here have been mostly fixed.

@eminence
Copy link
Contributor Author

I just did a quick test and things seem to be much better! The Window and Document types are fully resolved, and tab completion on both the win and doc variables seems to be working as expected. This is Very Cool. Thanks everyone! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macro macro expansion
Projects
None yet
Development

No branches or pull requests

9 participants