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 hint to "Unbound value .{}" error messages #6765

Closed
vicuna opened this issue Jan 30, 2015 · 12 comments
Closed

Add hint to "Unbound value .{}" error messages #6765

vicuna opened this issue Jan 30, 2015 · 12 comments

Comments

@vicuna
Copy link

vicuna commented Jan 30, 2015

Original bug ID: 6765
Reporter: @alainfrisch
Assigned to: @Octachron
Status: resolved (set by @Octachron on 2017-03-07T13:37:58Z)
Resolution: fixed
Priority: low
Severity: feature
Version: 4.03.0+dev / +beta1
Category: typing
Tags: patch
Monitored by: @gasche @hcarty @dbuenzli

Bug description

Now that bigarray index operators are defined in Bigarray, one needs to explicitly open this module to be able to use these operators (.{}, .{}<-, and -nary forms). To help in the transition, one could add a special case in the "Unbound value" error printer to suggest opening Bigarray when the name of the unbound value is one of the new operators.

Another approach to simplify porting big code bases would be to add a hack in the type-checker to force looking for these operators in Bigarray instead of failing with Unbound value, and issue a "Deprecated" warning when this is needed.

Ref:
#69

File attachments

@vicuna
Copy link
Author

vicuna commented Jan 30, 2015

Comment author: @gasche

I think we should go for the warning-only-for-now approach. Nicer to the user, and I don't think the patch required is large or would add maintenance cost.

@vicuna
Copy link
Author

vicuna commented Jan 30, 2015

Comment author: @hcarty

I would prefer the warning approach as well.

@vicuna
Copy link
Author

vicuna commented Jan 30, 2015

Comment author: @Octachron

I have implemented a rough version of the warning-only-for-now approach. The patch seems rather small and worked on few test cases.

@vicuna
Copy link
Author

vicuna commented Aug 14, 2015

Comment author: @dbuenzli

Also couldn't we have these index operators in a submodule opening Bigarray brings a lot of values in scope.

@vicuna
Copy link
Author

vicuna commented Aug 14, 2015

Comment author: @Octachron

I have been tempted for some time to propose a patch to add an Infix/Ops submodule to Bigarray/String/Bytes/Array (and maybe Map and Hashtbl?). For pratical use, it would probably be better if the submodule was included inside the main module, i.e.

module Bigarray = struct
...
module Infix = struct
...
end
include Infix
...
end
since like this both open Bigarray and open Bigarray.Infix would pull the infix operators in scope.

Has anyone any comment on this idea?

@vicuna
Copy link
Author

vicuna commented Aug 14, 2015

Comment author: @dbuenzli

I don't know if it's useful to do it for the other modules but at least for Bigarrays I think it should be done since it will be the only way to use them and I don't think opening Bigarray should be encouraged (even though the doc says so).

I was thinking that maybe we should try to come up with a better name than Ops/Infix for people to systematically use when they have a module which contains only indexing operators. The idea is that if I read that this is being opened I should pay attention to the semantics of .() and .[] in order to figure out whatever imperative crap may be hidden behind this (I personally find this indexing operator redefinition a very dubious idea).

I propose Indexing. If the design pattern is given a module M you have M.Indexing and Indexing included in M. Then you can use open M.Indexing at the toplevel of a file for global opens and M.() for more localized indexing access.

@vicuna
Copy link
Author

vicuna commented Aug 15, 2015

Comment author: @Octachron

since it will be the only way to use them and I don't think opening Bigarray should be encouraged (even though the doc says so).

In the current trunk version, the indexing operators are standard operator identifiers, so it is perfectly possible to fetch only these operators from the Bigarray module:

let (.{}), (.{}<-) = Bigarray.( (.{}), (.{}<-) );;

Having an indexing submodule would be only for convenience reason.

I should pay attention to the semantics of .() and .[]

Does it means that you would prefer to have different submodule names for module redefining respectively the .() / .[] / .{} operator family?

whatever imperative crap may be hidden behind this

I don't see much difference here compared to any other functions or operators.

@vicuna
Copy link
Author

vicuna commented Sep 27, 2015

Comment author: @Octachron

To continue the discussion on an eventual indexing submodule in the bigarray library, I have submitted a pull request here #248.

@vicuna
Copy link
Author

vicuna commented Nov 18, 2015

Comment author: @alainfrisch

If we go for the improved error message only, I have submitted #299

@vicuna
Copy link
Author

vicuna commented Nov 27, 2015

Comment author: @alainfrisch

Customizable indexing operator won't be in 4.03: postponing this as well.

@vicuna
Copy link
Author

vicuna commented Mar 7, 2017

Comment author: @mshinwell

I have a feeling this could be closed now.

@vicuna vicuna closed this as completed Mar 7, 2017
@vicuna
Copy link
Author

vicuna commented Mar 7, 2017

Comment author: @Octachron

I agree that this phantom issue should be closed.

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

No branches or pull requests

2 participants