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

Add a diagnostic for unused imports in nargo language server #4762

Closed
LogvinovLeon opened this issue Apr 9, 2024 · 2 comments · Fixed by #5847
Closed

Add a diagnostic for unused imports in nargo language server #4762

LogvinovLeon opened this issue Apr 9, 2024 · 2 comments · Fixed by #5847
Labels
enhancement New feature or request help wanted Contributions welcome, read CONTRIBUTING.md for how-to lsp Language Server Protocol

Comments

@LogvinovLeon
Copy link
Contributor

LogvinovLeon commented Apr 9, 2024

Problem

When refactoring Noir code, developers face the challenge of manually adjusting imports, as there is currently no automated way to generate required imports or remove unused ones. This manual process results in tedious and error-prone import management.

Noir Docs
Example Noir Code with Modules

Happy Case

The Noir VSCode extension has the potential to alleviate this problem by communicating with the Nargo language server. This communication enables the extension to provide import-related diagnostics and actions, streamlining the import management process for developers.

Project Impact

While not critical, integrating automated import management into the Noir VSCode extension would greatly enhance the development experience by reducing manual effort and improving code cleanliness.

Impact Context

Without automated import management, developers must spend significant time and effort managing imports manually during refactoring, which hampers productivity and introduces the risk of errors.

Workaround

None

Workaround Description

No response

Additional Context

Noir already supports unused variables diagnostic and it's very useful

Existing Functionality

Noir already has a language server implemented, residing in the repository under tooling/lsp. This server provides some functionality, such as reporting unused variables, which are highlighted in the IDE by the Noir extension. However, it currently does not report unused imports. Adding support for this functionality is the focus of this issue.

Proposal for Resolution

Integrate automated import management features into the Noir VSCode extension, leveraging the capabilities of the existing Noir language server. This includes adding support for reporting and addressing unused imports, similar to the existing functionality for unused variables.

Would you like to submit a PR for this Issue?

Maybe

Support Needs

While the implementation of this feature is feasible, it may require collaboration with the community and feedback from project maintainers to ensure alignment with project goals and standards.

@LogvinovLeon LogvinovLeon added the enhancement New feature or request label Apr 9, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Apr 9, 2024
@Savio-Sou Savio-Sou added the lsp Language Server Protocol label Apr 10, 2024
@Kapple14
Copy link

Additional Context:
Noir already implemented a language server, residing in the repository under tooling/lsp. This language server adheres to the Language Server Protocol (LSP), a standardized protocol for communication between code editors and language servers. While the Noir language server provides some functionality, such as reporting unused variables, currently it cannot report unused imports. This means that while unused variables are highlighted in the IDE by the Noir extension, unused imports are not flagged. Adding support for detecting and managing unused imports through the Noir language server is crucial for enhancing the refactoring experience within the Noir codebase.

@Savio-Sou Savio-Sou added the help wanted Contributions welcome, read CONTRIBUTING.md for how-to label May 15, 2024
@Savio-Sou
Copy link
Collaborator

This would be a great DevEx add 👍

If anyone is interested in contributing, feel free to create a PR / comment below.
Happy to provide support, answer questions and review works 💪

github-merge-queue bot pushed a commit that referenced this issue Aug 28, 2024
# Description

## Problem

Resolves #4762

## Summary

The compiler now produces a warning on unused imports.

## Additional Context

I don't know if this is the correct approach. I added a new HashSet to
track imported names, then those are removed as we import things. I
thought about tracking this in `ItemScope` but it's tricky because the
way things are they include imported and self things.

There's another thing: eventually it would be nice to warn on unused
types, like in Rust. For that maybe it would make sense to track this in
`ItemScope`... but given that we currently don't have visibility
modifiers for types, it can't be done right now. So maybe doing it just
for imports with specific code is fine.

I had to refactor a bit `resolve_path` because some paths were looked up
not using that method and so some things weren't marked as used... but
now `StandardPathResolver::new` is used in exactly one place in the
compiler.

## Documentation

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Contributions welcome, read CONTRIBUTING.md for how-to lsp Language Server Protocol
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants