Skip to content

Remove repetitive inlay hints #6115

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

Closed

Conversation

matthewsanetra
Copy link
Contributor

This removes/fixes unnecessary hints when the function name ends with the first param name, and increases readability.

Example:

s.strip_suffix(suffix: "Hello");

would now become

s.strip_suffix("Hello");

Happy Hacktoberfest! 🎉

@lnicola
Copy link
Member

lnicola commented Oct 1, 2020

I'm surprised at the amount of heuristics that we've amassed here.

In this specific case, though, I'm worried about something like fn max(x: u32, y: u32). Maybe we should check that the function name matches the parameter name, or ends in _ followed by it? Also, this could probably use a test.

And thank you for the PR!

@matthewsanetra
Copy link
Contributor Author

Oh wow, I didn't think about the fn max(x: u32, y: u32) scenario, I think the simplest way to fix this is to check if the fn_name ends with param_name prepended with _.

However, since single letter parameters aren't used unless its obvious, maybe we also just don't display single letter params?

@lnicola
Copy link
Member

lnicola commented Oct 1, 2020

I think the simplest way to fix this is to check if the fn_name ends with param_name prepended with _.

Maybe? There might be functions with the same name as their arguments, but I can't think of an example.

However, since single letter parameters aren't used unless its obvious, maybe we also just don't display single letter params?

Maybe? 😆

@matthewsanetra
Copy link
Contributor Author

I am extremely sorry for accidentally closing it, I'm going to remove the unnecessary allocation now.

@Veetaha
Copy link
Contributor

Veetaha commented Oct 2, 2020

I think to be more flexible we should just expose a configuration for a list of regular expressions with call patterns like it is done in Intelij
image

@Veetaha
Copy link
Contributor

Veetaha commented Oct 6, 2020

I didn't mean to stall this PR, it is definitely useful now. My comment is more about possible far future changes =)

@lnicola
Copy link
Member

lnicola commented Oct 6, 2020

I think the heuristics work better because nobody has to compile a list of "obvious function signatures", not to mention that different people would have different notions of obviousness wrt. e.g. strncmp.

@matthewsanetra
Copy link
Contributor Author

matthewsanetra commented Oct 6, 2020

expose a configuration for a list of regular expressions with call patterns

Wouldn't that be best to do in an extension/client side?
To switch over you would need to:

  • Encourage every r-a extension out there to add this functionality
  • Slowly phase out heuristics inside the language server, making sure that users don't see any difference in user experience while this is happening.

@matthewsanetra
Copy link
Contributor Author

CI is failing when testing proc_macro_srv, exiting with STATUS_ILLEGAL_INSTRUCTION. Any help?

@lnicola
Copy link
Member

lnicola commented Oct 8, 2020

Try to rebase over master.

@matthewsanetra matthewsanetra force-pushed the better_param_inlays branch 4 times, most recently from 6434949 to 5377c5a Compare October 8, 2020 20:10
@matthewsanetra
Copy link
Contributor Author

Hey -- I'm sorry, I'm new to contributing. I tried to rebase as with the help provided by the check_merge_commits check, but now it's saying conflicting files, and if I try to resolve that on GitHub the check fails. Any guidance?

@SomeoneToIgnore
Copy link
Contributor

Hey, I've checked your branch and looks like you have the conflicts with master branch indeed:

both modified:   crates/ide/src/inlay_hints.rs
both modified:   crates/ide/src/references.rs
added by us:     crates/ide_db/src/apply_change.rs

Since you seem to have a single new commit in you branch, I suggest you to first pull the latest changes for the rust-analyzer's master branch and then rebase on top of it, fixing the conflicts for all 3 files.
Then you can force push into your remote branch to propagate the changes to the server.

matthewsanetra and others added 7 commits October 10, 2020 00:54
* Add convert integer literal assist

* Add ability to specify ResolvedAssist by label

* Add tests for convert integer literal assist

* Minor clippy performance suggestions

* Fix path comparison not comparing paths correctly with unequal lengths

* Improve grammar and fix code example in style guide

* Up rustc-ap-rustc_lexer to 681

cargo update as well

* Document Clippy strategy

* Add support of runnables arguments in Rust Analyzer

* Replace 'cargo_prefix' option with 'override_cargo'

* Support 'runnables' options in the vs code extension

* Fix failing test

* Add postfix completion for format-like string literals

* Improve checks for postfix suggestions

* Simplify is_string_literal function

* Add missing entry to doc-comment

* Improve format-like completions code appearance

* Use ast::String for extracting string literal contents

* Use lookup table instead of enum for postfix completion kinds

* Use expect_test to make format_str_parser test more data-driven

* Reduce visibiity

* Expectify find_references tests

* Move ide::AnalysisChange -> base_db::Change

This seems like a better factoring logically; ideally, clients shouldn't touch
`set_` methods of the database directly. Additionally, I think this
should remove the unfortunate duplication in fixture code.

* Rewrite fixtures on top of Change

* Get rid of MockAnalysis

* rename mock_analysis -> fixture

* Add a dash test

* Fix the hover dash issues

* Properly name the field

* Simplify ast_transform

* Correctly complete items with leading underscore

* Better inlay hints in 'for' loops

* Make the tests for complete/incomplete for inlay hints work

* Remove 'for_expr' test from inlay_hints.rs

* Improve readability in inlay_hints.rs

* Trim all trailing whitespace in onEnter

Fixes rust-lang#5848

* Add notes concerning privacy and network access

* Make find_path_prefixed configurable

* Make ImportPrefix a configuration option

* honor content_format clientcap

This removes all markdown when the client does not support the markdown MarkupKind

Otherwise the output on the editor will have some markdown boilerplate, making it less readable

* add docstring

* Fix feature name

* Account for proc macro helpers when parsing attr

* Do not leave braces for colons in dbg!

* Fix trait object hir formatting behind pointer and references

* Smoke test docs generation

* Move ModPath->ast::Path function to IDE layer

closes rust-lang#6092

* add doc describing limited capabilities

* add break after codeblocks

* Constrain ImportMap to only store simple paths

* Shorten type hints for std::iter Iterators

* Add test makr

* Document privacy invariant of SyntaxPtr

* Use FamousDefs for shorten_iterator hint

* Fixed parsing of negative number literals in macros.

* Added unit test for negative number literals in macros.

* Update manual.adoc

* Add validation check for ambiguous trait objects

* Bump chalk to use latest git to get fix

 * Chalk very recently (like an hour ago) merged a fix that prevents rust analyzer from panicking.  This allows it to be usable again for code that hits those situations.  See rust-lang#6134, rust-lang#6145, Probably rust-lang#6120

* `todo!()` -> `unimplemented!() // FIXME` for CI

* Make unimplemented match variants explicit

* Move IntoIterator into FamousDefs

* Shorten iterator chain hints

* Cleanup

* Shorten iterator hints for std::iter iterators behind references

* Better progress API

Percentage is a UI concern, the physical fact here is fraction. It's
sad that percentage bleeds into the protocol level, we even duplicated
this bad API ourselves!

* Reorg style

* Add comparisons guideline to style

* minor

* minor

* Clean up inlay_hints

* Switch from git to latest tagged release of chalk deps

* Add track_env_var to the proc macro server

* Bump rustc_lexer, cfg-if to 1.0 and add new license to check

* add eprintln in fmt-like postfix

Signed-off-by: Benjamin Coenen <[email protected]>

* Fix source_to_def for named enum variant fields

* Add note if RUST_SRC_PATH is likely to be wrong

* Update crates/project_model/src/sysroot.rs

Co-authored-by: Laurențiu Nicola <[email protected]>

* Remove release build overrides

* Treat `ast::Name` in field patterns as use

* adt: correctly inherit field visibility from enum

Previously, "find all references" on a variant field wouldn't find any
references outside the defining module. This is because variant fields
were incorrectly assumed to be private, like struct fields without
explicit visibility, but they actually inherit the enum's visibility.

Co-authored-by: vlakreeh <[email protected]>
Co-authored-by: kjeremy <[email protected]>
Co-authored-by: Lukas Wirth <[email protected]>
Co-authored-by: Laurențiu Nicola <[email protected]>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
Co-authored-by: Aleksey Kladov <[email protected]>
Co-authored-by: Igor Aleksanov <[email protected]>
Co-authored-by: Kirill Bulatov <[email protected]>
Co-authored-by: León Orell Valerian Liehr <[email protected]>
Co-authored-by: Wesley Norris <[email protected]>
Co-authored-by: Robin van Dijk <[email protected]>
Co-authored-by: Jonas Schievink <[email protected]>
Co-authored-by: Tim <[email protected]>
Co-authored-by: Adrian Stanciu <[email protected]>
Co-authored-by: Casey Primozic <[email protected]>
Co-authored-by: Benjamin Coenen <[email protected]>
Co-authored-by: Laurențiu Nicola <[email protected]>
@matthewsanetra
Copy link
Contributor Author

Hi, Sorry @SomeoneToIgnore , I just did what you suggested, but then the build was failing as an import was missing, I just added that in the fix build commit, and now there's a conflict again. I'm really sorry, why does this happen?

@SomeoneToIgnore
Copy link
Contributor

There can be many reasons: you could have not actually updated the master branch before resolving the conflicts, there can be new changes in that branch after you resolution, there can be something else I'm not aware of.

Anyway, you have to resolve those conflicts, so use the same approach again until they are gone.
If the merge conflicts do not disappear after a couple attempts, then you're doing something wrong and it'a a good time to stop and think thoroughly.

@matklad
Copy link
Member

matklad commented Oct 12, 2020

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

Successfully merging this pull request may close these issues.

6 participants