Skip to content
This repository has been archived by the owner on Jan 25, 2024. It is now read-only.

improved unbound identifier detection #95

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

symphorien
Copy link
Contributor

followup to #88

parse all of rnix typed nodes so that unbound identifiers can be detected in much more cases

the evaluation machinery only returns unimplemented for the new nodes

image

at some point I ran rustfmt on the project by habit, I tried to undo some of the changes, but the diff is not minimal.

@symphorien symphorien changed the title Static improved unbound identifier detection Jul 23, 2022
src/eval.rs Show resolved Hide resolved
src/eval.rs Show resolved Hide resolved
"false" => NixValue::Bool(false),
"null" => NixValue::Null,
// list obtained with `nix repl` tab completion
"abort"
Copy link
Member

Choose a reason for hiding this comment

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

Where do you recommend that we actually define these values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. Maybe we can ignore everything that starts with __, accept the builtins and hardcode the 3 or 4 remaining cases like true and false ?

src/tests.rs Show resolved Hide resolved
src/eval.rs Outdated
/// whether the patter is incomplete (contains `...`)
ellipsis: bool,
/// the identifier bound by `@`
at: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
at: Option<String>,
at: Option<ExprResultGc>,

This would let us put the identifier in a Scope, and the Expression struct would also give us more info about where the identifier is located.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it an ExprResultBox as there is no need for sharing as far as I can see

@aaronjanse
Copy link
Member

@symphorien Would you mind if I merge in the ExprSources alongside an rnix-parser update in a separate PR? I'd put all your edits in a commit, with you marked as the author and me as the committer, so you'd get credit in the Github UI.

Updating rnix-parser would be easier with some of the definitions you added, like StringPartSource.

@symphorien
Copy link
Contributor Author

Fine by me as long as it does not create extra merge conflicts, I have a branch with unused binding detection at https://github.com/symphorien/rnix-lsp/tree/unused and it's based on this branch. (will make a PR after some more testing)

@symphorien
Copy link
Contributor Author

ping @aaronjanse

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

Successfully merging this pull request may close these issues.

None yet

2 participants