Comment author: @gasche

I plead guilty as a this could be a spellchecking-related bug. I'll have a look in the week.

Comment author: @alainfrisch

Fixed by commit 13214. Gabriel: do you agree with the fix? I don't think that the Env.fold_* functions should ever raise Not_found.

Comment author: @gasche

Thanks for tackling the issue so quickly!

My understanding of the problem is the following (sorry for repeating this to you Alain, but it may be an useful point of reference if someone else stumbles on this).
The Env.find_all function(s) has an option to refine folding, not to all the environment but to only the identifiers 'id' accessible under a given module longindent 'L' (that is directly L.id, with no search in the submodules of L). It expects that the longident is correct, in that it correctly returns a module component. In the present case, the longident is incorrect: it denotes an unapplied functor rather than a module.

Your fix effectively mutes the error in this case. Another option would be to check before calling Env.find_all (in fact the Env.fold_* functions called at spellchecking time) that the longident is valid. I think that would be nicer from an user point of view: during the narrow_unbound_lid_error step (that refines an unbound-foo error by finding the shortest wrong prefix of the component), we could check in the Ldot(mlid, _) case that mlid, when it exists, indeed denotes a module component.

This would require adding a new kind of error for this. Do you think it is acceptable? (I'm ready to try to contribute a patch if you don't wish to invest in refining this.)

However, both in this case and in the Lapply situation of #5046, I do not really understand why this is handled as a lexical error rather than a typing error. I would rather expect a Typemod error rather than a Typetexp error.

I think do not understand the typer code well enough to suggest a change there; it seems that the Typetexp.find_foo functions is called precisely when we only have a longindent rather than a Path.t (already type-checked), as in the latter case Env.find_foo (which uses no spell-checking logic) is preferred. What is the logic for using Typetexp instead of typechecking the path into a Path.t first, and why does it suffices in practice to do spell-checking on Typetexp only (are Env.find_* functions assumed never to fail?), I don't know.

PS: When I contributed the spell-checking patch I found the Env.fold_* functions and was delighted to use them; I believed that there had been there for a long time. I looked after your fix, and they apparently come from the huge bin-annot blurb patch, and are seemingly never used anywhere expect in the spellchecking logic. I was right to take care to flush the error buffer before doing the spell-checking, but for the wrong reason: the problem is not that spellchecking may take time, but that it calls non-battle-tested code and is therefore prone to be buggy.

Comment author: @alainfrisch

Yes, I believe it would make sense to have a proper error message in that case.