nixd icon indicating copy to clipboard operation
nixd copied to clipboard

Missing detection of `__findFile` usages.

Open voronind-com opened this issue 1 year ago • 10 comments

Describe the bug Right now nixd reports __findFile as unused even when there are <> references.

Expected behavior Using <path> references marks __findFile arg as used.

Screenshots image

voronind-com avatar Nov 23 '24 05:11 voronind-com

I think __findFile is internal to C++ nix implementation thus we should not expose this explicitly. CC @roberth @Mic92

inclyc avatar Nov 23 '24 06:11 inclyc

I think __findFile is internal to C++ nix implementation thus we should not expose this explicitly. CC @roberth @Mic92

Why not? Is it gonna hurt users with other implementations? Because now it hurts people with cppnix, which are the majority.

Also this might only mean that nixd could use another configuration parameter being the nix implementation used.

People without cppnix would not use this thing in their codebase anyway. And this thing is quite popular.

voronind-com avatar Nov 23 '24 07:11 voronind-com

Please note that the language reference only states that

A lookup path is an identifier with an optional path suffix that resolves to a path value if the identifier matches a search path entry in builtins.nixPath. The algorithm for lookup path resolution is described in the documentation on builtins.findFile.

But it never said <> expressions will be expanded to __findFile. It just said: it is guaranteed that "the algorithm" is the same as builtins.findFile.

So I don't think it is correct to treat this syntax as a user of __findFile.

Why not? Is it gonna hurt users with other implementations? Because now it hurts people with cppnix, which are the majority.

It is not a consideration about implementations, but about what is "exposed" from a language's specification.

inclyc avatar Nov 23 '24 07:11 inclyc

Do you mean this is about usability vs correctness?

voronind-com avatar Nov 23 '24 08:11 voronind-com

This is the first time, I hear of __findFile. Why do user have to specify it explicitly and how is it useful?

Mic92 avatar Nov 23 '24 08:11 Mic92

This is the first time, I hear of __findFile. Why do user have to specify it explicitly and how is it useful?

Actually nix does allow overriding many base "internal implementation". For example,

a < b

will be expanded into

__lessThan a b

And in fact users can redefine __lessThan, e.g.

❯ nix repl
Nix 2.24.10
Type :? for help.
nix-repl> __lessThan = x: y: x + y

nix-repl> 1 < 2
3

inclyc avatar Nov 23 '24 08:11 inclyc

Oh. We shouldn't users encourage to do that I suppose. It's ok for debugging tools to show these internals, but endusers should not write their code in this way. If it would help to resolve the issue, maybe it could be turned into a warning that an internal symbol is used?

Mic92 avatar Nov 23 '24 08:11 Mic92

Oh. We shouldn't users encourage to do that I suppose. It's ok for debugging tools to show these internals, but endusers should not write their code in this way. If it would help to resolve the issue, maybe it could be turned into a warning that an internal symbol is used?

Agreed. Hi @voronind-com, would you like to accept this solution? i.e. let nixd mark warnings at internal symbols usages.

inclyc avatar Nov 23 '24 08:11 inclyc

Oh. We shouldn't users encourage to do that I suppose. It's ok for debugging tools to show these internals, but endusers should not write their code in this way. If it would help to resolve the issue, maybe it could be turned into a warning that an internal symbol is used?

Agreed. Hi @voronind-com, would you like to accept this solution? i.e. let nixd mark warnings at internal symbols usages.

I'm unsure. The __findFile is commonly used as an easy reference from the git root, i.e. <packages> translates to the /packages dir.

I still vote that __findFile should be considered used (as it technically is) when there are <>. As well as __lessThan when < is used etc.

Should warnings be added on top of that or not is another question.

voronind-com avatar Nov 23 '24 09:11 voronind-com

Well probably fair. I am just not sure how hard to special case it is nice.

Mic92 avatar Nov 23 '24 13:11 Mic92