Add (:include <file>) terms to include_dir field

[gridbugs]

Addresses #3993

Still need to add a version number check but opening this now to get preliminary feedback while I work out the best way to do that.

[rgrinberg]

This feature is introduced in 3.5, therefore it should only be available when 3.5 is enabled. See some example in dune_file.ml on how to do that.

[gridbugs]

Is it correct that if an older version is enabled, dune should behave as it did prior to this feature being added, and not explicitly raise an error indicating the minimum version supporting this feature?

[rgrinberg]

Yes, that is correct. However, if previous versions would reject (:include ..) as an unexpected list, here we should hint to the user that this feature is available starting from 3.5.

[rgrinberg]

Looks good in general. Design wise, one thing that bothers me is that we're enabling :include but not the whole ordered set language in this field. That is inconsistent with how dune works elsewhere. Can we try and support the full ordered set language here instead?

[gridbugs]

I totally agree with your point about inconsistency. I don't think there's a way to treat the entire (include_dirs ...) field as an ordered set because it currently allows terms of the form (lib <libname>) which isn't valid OSL syntax. An alternative would be to allow terms that introduce OSL expressions, e.g.:

(include_dirs (osl (:include foo)))

[gridbugs]

Also, my current implementation makes it an error if the file referred to by (:include ...) contains an OSL with 0 or >1 elements. It wouldn't be hard to support multiple directories referred to by a single (include ...)-ed file, but I'm not sure if that's ever something that will be useful in practice. @rgrinberg any thoughts on this?

[rgrinberg]

I totally agree with your point about inconsistency. I don't think there's a way to treat the entire (include_dirs ...) field as an ordered set because it currently allows terms of the form (lib ) which isn't valid OSL syntax. An alternative would be to allow terms that introduce OSL expressions, e.g.:

It should still be possible to support. We can instantiate ordered set lang with other ast elements. In this case, it would be with this type:

    type t =
      | Dir of String_with_vars.t
      | Lib of Loc.t * Lib_name.t

You might need to duplicate some stuff from Ordered_set_lang.Unexpanded.t because our stuff isn't well factored, but it should still be doable.

[rgrinberg]

Also, my current implementation makes it an error if the file referred to by (:include ...) contains an OSL with 0 or >1 elements. It wouldn't be hard to support multiple directories referred to by a single (include ...)-ed file, but I'm not sure if that's ever something that will be useful in practice. @rgrinberg any thoughts on this?

It's not particularly useful, but it does make it consistent with other uses of (:include ..) so I think it's worth doing.

[gridbugs]

@rgrinberg based on our discussion I've redesigned this change so that the include_dirs field can contain an arbitrary ordered set of Include_dir.ts. I factored out the common logic in Ordered_set_lang.Unexpanded into a functor. I still need to make behaviour fall back to pre-3.5 when the version is <3.5 but updating the PR for early feedback.

[rgrinberg]

In our convention, the _intf suffix is reserved for .ml only modules that contain module singatures (whenever we don't want to duplicate the signature in the mli). Module types never have this suffix so this should be called Element.

[rgrinberg]

Therefore, if if you prefer, you can move Ordered_set_lang.Element to Ordered_set_lang_intf.Element to avoid writing the module signature twice.

[rgrinberg]

Should be type expanded := t and you can remove the destructive substitution in the end.

[rgrinberg]

This suggestion is more subjective so feel free to ignore/delay it:

It's a little weird that a general purpose thing such as Ordered_set_lang should depend on Include_dir. Therefore, I would expect Include_dirs to live in a separate module, or at least as a submodule of Include_dir.

[gridbugs]

I agree in principle, but making that change would require exposing more of the inner working of Ordered_set_lang to the outside world, and that feels like a bigger change.

[rgrinberg]

The PR looks good. I left some minor comments that should be trivial to address. Before we merge this PR, you should also:

• Update the change log at CHANGES.md
• Update the manual.

[rgrinberg]

From what I understand, (include ..) is allowed to expanded into OSL, right? That seems counterintuitive to me because OSL isn't accepted in the field when it is statically interpreted. Furthermore, the following isn't allowed either:

$ cat foo
  (baz (lib foo))
$ cat >dune << EOF
> ..
> (include_dirs (include foo))
> EOF

Which I would expect to be allowed.

[gridbugs]

Good point. I think it was a mistake to use OSL for this feature. I'll change so that the file specified to (include ...) can contain a sexp in the same notation as the (include_dirs ...) field.

[rgrinberg]

Yes indeed. It was a nice idea, but it doesn't seem to work out in practice.

[gridbugs]

I've updated this PR such that now the contents of the included file is parsed using the (include_dir ...) parser

[christinerose]

Suggested change

	Warning: (include ...) in include_dirs field is only available since version
	3.5 of the dune language. Please update your dune-project file to have (lang
	dune 3.5).
	Warning: (include ...) in ``include_dirs`` field is only available since version
	3.5 of the Dune language. Please update your ``dune-project`` file to have (lang
	dune 3.5).

[gridbugs]

I've quoted "include_dirs". The rest of the suggested changes here are in text generated by Syntax.Error_msg.since.

[rgrinberg]

CI says we need a promotion here.

[gridbugs]

I forgot to update a test when I changed an error message. That's fixed now, and I've also split the tests into multiple files with one file per test case.

[rgrinberg]

Looks good. I merged with a few tweaks.