Skip to content

Add support for include_str #5100

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 27, 2020
Merged

Add support for include_str #5100

merged 1 commit into from
Jun 27, 2020

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jun 27, 2020

@edwin0cheng
Copy link
Member

Maybe we could reuse https://github.com/rust-analyzer/rust-analyzer/blob/6a067ce947980d887a254d71d7183c3d306d8978/crates/rust-analyzer/tests/heavy_tests/main.rs#L468 and add a test ?

@lnicola
Copy link
Member Author

lnicola commented Jun 27, 2020

I'm not sure what to test for? For some reason, "expand macros recursively" doesn't seem to work.

@edwin0cheng
Copy link
Member

Adding something like this inside the out_dir_checks should works:

let should_<|>be_str = include_str!(“text.rs”);

@lnicola
Copy link
Member Author

lnicola commented Jun 27, 2020

For some reason, we don't seem to find the file -- relative_file(db, arg_id.into(), &path) is None even though path looks correct. This happens even with include!("main.rs"), but we might not find the file in the database anyway. Should we expand to "" for now?

@edwin0cheng
Copy link
Member

iirc, we added a constraint for limiting self including in relative_path in include! case. Maybe we should add an enum to turn off it if needed ?

@lnicola
Copy link
Member Author

lnicola commented Jun 27, 2020

Oh, right, it works with other files. But what about non-.rs files? I assume they're not going to be found in the database.

@edwin0cheng
Copy link
Member

edwin0cheng commented Jun 27, 2020 •
edited
Loading

I remember we have some discussion with @matklad that all non-rs files should be included by users explicitly. So I think it is okay for now

@lnicola
Copy link
Member Author

lnicola commented Jun 27, 2020

Added a small hack.

@edwin0cheng
Copy link
Member

Just small nits, otherwise LGTM

bors d+

@bors
Copy link
Contributor

bors bot commented Jun 27, 2020

✌️ lnicola can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@lnicola
Copy link
Member Author

lnicola commented Jun 27, 2020

Just small nits, otherwise LGTM

Which nits? 😄

edwin0cheng
edwin0cheng reviewed Jun 27, 2020
View reviewed changes
crates/rust-analyzer/tests/heavy_tests/main.rs Outdated
),
work_done_progress_params: Default::default(),
});
assert!(dbg!(res.to_string()).contains("&str"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remain dbg ?

@lnicola
Copy link
Member Author

lnicola commented Jun 27, 2020

bors r=edwin0cheng

@bors
Copy link
Contributor

bors bot commented Jun 27, 2020

Build succeeded:

@bors bors bot merged commit 446fd3f into rust-lang:master Jun 27, 2020
@lnicola lnicola deleted the include-str branch June 27, 2020 16:46
@lnicola lnicola restored the include-str branch June 27, 2020 16:46
@lnicola lnicola deleted the include-str branch June 27, 2020 16:46
@edwin0cheng edwin0cheng mentioned this pull request Jun 29, 2020
Closed
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwin0cheng edwin0cheng edwin0cheng left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lnicola @edwin0cheng