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

Remove (.[]<-) #11345

Merged
merged 7 commits into from
Jun 24, 2022
Merged

Remove (.[]<-) #11345

merged 7 commits into from
Jun 24, 2022

Conversation

nojb
Copy link
Contributor

@nojb nojb commented Jun 21, 2022

This operator should have been removed when the function String.set was removed in #10867.

Fixes #11342

@xavierleroy
Copy link
Contributor

The only potential downside I see to this change is that it rules out the following valid OCaml 4 code:

module BytesNotation = struct
    module String =
      let get = Bytes.get
      let set = Bytes.set
    end
  end;;
# let b = Bytes.make 25 'x';;
val b : bytes = Bytes.of_string "xxxxxxxxxxxxxxxxxxxxxxxxx"
# BytesNotation.(b.[2] <- 'y');;
- : unit = ()
# BytesNotation.(b.[2]);;
- : char = 'y'

I don't really want support this kind of abuse of notations, however.

@Octachron
Copy link
Member

Do we want a specialized error message? It is quite exceptional that the removal of a deprecated function might result in a syntax error in user code. It might be help confused users to have a specific explanation rather than just a Error: Syntax error located on the assignment arrow <- .

@xavierleroy
Copy link
Contributor

I agree that a specialized error message would be nice, at least in 5.0. It could be removed in later releases.

@nojb
Copy link
Contributor Author

nojb commented Jun 21, 2022

I agree that a specialized error message would be nice, at least in 5.0. It could be removed in later releases.

I gave this a try, it looks like this:

$ TERM=dumb ocaml
OCaml version 5.1.0+dev1-2022-06-09
Enter #help;; for help.

# s.[i] <- x;;
Line 1, characters 0-10:
1 | s.[i] <- x;;
    ^^^^^^^^^^
Error: the infix operator (.[]<-) used to be bound to String.set, which has been removed in OCaml 5.0

@dbuenzli
Copy link
Contributor

Error: the infix operator (.[]<-) used to be bound to String.set, which has been removed in OCaml 5.0

The message doesn't say what the problem is. You should say it is illegal syntax.

@nojb
Copy link
Contributor Author

nojb commented Jun 21, 2022

Error: the infix operator (.[]<-) used to be bound to String.set, which has been removed in OCaml 5.0

The message doesn't say what the problem is. You should say it is illegal syntax.

OK, I tweaked the message:

$ TERM=dumb ocaml
OCaml version 5.1.0+dev1-2022-06-09
Enter #help;; for help.

# s.[i] <- c;;
Line 1, characters 0-10:
1 | s.[i] <- c;;
    ^^^^^^^^^^
Error: Syntax error: the infix operator (.[]<-) used to be bound to String.set, which was removed in OCaml 5.0

s.[i + 1] <- Char.chr (n lsr 16 lor 0xff);
s.[i + 2] <- Char.chr (n lsr 8 lor 0xff);
s.[i + 3] <- Char.chr (n lor 0xff);
s.![i] <- Char.chr (n lsr 24);
Copy link
Member

Choose a reason for hiding this comment

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

I’m a bit surprised by this change. Where is the (.![]<-) operator defined? I wasn’t able to locate it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test only checks that the source file can be parsed.

Copy link
Member

Choose a reason for hiding this comment

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

ah I see. Thanks!

@Octachron
Copy link
Member

Octachron commented Jun 22, 2022

I am not that fond of having history-focused user messages. When reading

 Syntax error: the infix operator (.[]<-) used to be bound to String.set, which was removed in OCaml 5.0

a new user will have to search what was String.set in older versions of OCaml, understand why it existed, then why it was removed before finally making the link with their current issue.

Even regular users might scratch their head about the information that .[]<- was bound to String.set (or String.unsafe_set) which was always an implementation detail.

I would propose to focus on the current situation:

  • strings are immutable
  • an assignment operator makes no sense for immutable data types
  • there is a mutable sequence of bytes data types: Bytes.t

Something like

Syntax error, strings are immutable, there are no assignment syntax for them.
Hint: Mutable sequence of bytes are available in the Bytes module.

but I am missing a good way to bring up Bytes.set. And there is some potential confusion for people that knowingly ignored the deprecation warning for .[]<- and got used to the idea that .[]<- worked on bytes rather than strings.

@nojb
Copy link
Contributor Author

nojb commented Jun 22, 2022

Syntax error, strings are immutable, there are no assignment syntax for them.
Hint: Mutable sequence of bytes are available in the Bytes module.

Thanks, this feels much better. I amended the patch:

OCaml version 5.1.0+dev1-2022-06-09
Enter #help;; for help.

# s.[i] <- c;;
Line 1, characters 0-10:
1 | s.[i] <- c;;
    ^^^^^^^^^^
Error: Syntax error: strings are immutable, there is no assignment syntax for them.
Hint: Mutable sequences of bytes are available in the Bytes module.
#

@xavierleroy xavierleroy added this to the 5.0 milestone Jun 24, 2022
Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nojb nojb changed the base branch from 5.0 to trunk June 24, 2022 08:36
@nojb
Copy link
Contributor Author

nojb commented Jun 24, 2022

Looks good to me.

Thanks. I rebased on trunk (with the Changes entry on 5.0 section). After merging I will cherry-pick to the 5.0 branch.

@nojb nojb closed this Jun 24, 2022
@nojb nojb reopened this Jun 24, 2022
@gasche
Copy link
Member

gasche commented Jun 24, 2022

Hint: Mutable sequences of bytes are available in the Bytes module.
Instead of `s.[i] <- c`, one may write `Bytes.set buf i c`.

(I'm not suggesting to pretty-print the actual subterms of the user, that sounds like a dubious idea, but I think showing some concrete syntax can help.)

@nojb
Copy link
Contributor Author

nojb commented Jun 24, 2022

Hint: Mutable sequences of bytes are available in the Bytes module.
Instead of `s.[i] <- c`, one may write `Bytes.set buf i c`.

(I'm not suggesting to pretty-print the actual subterms of the user, that sounds like a dubious idea, but I think showing some concrete syntax can help.)

Personally feels a bit overkill to me, but am not opposed. If no-one voices an objection, I'll amend accordingly.

@gasche
Copy link
Member

gasche commented Jun 24, 2022

I think that for beginners there is a certain distance between "use the module Bytes" and (1) finding the Bytes documentation then (2) figuring out that Bytes.set is the right function and (3) how to use it. Seeing the concrete code can save a bit of time.

@Octachron
Copy link
Member

I agree that we want to mention Bytes.set. Do we need to refer to the previous syntax, however

Hint: Mutable sequences of bytes are available in the Bytes module.
Hint: Did you mean to use `Bytes.set buffer index char`?

?

@nojb
Copy link
Contributor Author

nojb commented Jun 24, 2022

I think that for beginners there is a certain distance between "use the module Bytes" and (1) finding the Bytes documentation then (2) figuring out that Bytes.set is the right function and (3) how to use it. Seeing the concrete code can save a bit of time.

OK, I pushed a commit as suggested.

@nojb
Copy link
Contributor Author

nojb commented Jun 24, 2022

Hint: Did you mean to use Bytes.set buffer index char?

What about

Hint: Did you mean to use 'Bytes.set'?

?
(otherwise I'm afraid that buffer, index, char may end up confusing rather than clarifying)

@Octachron
Copy link
Member

Yes, your version seems clearer to me.

@dbuenzli
Copy link
Contributor

(otherwise I'm afraid that buffer, index, char may end up confusing rather than clarifying)

Agreed. If you don't reply with the names that are in the context it is going to be confusing.

@nojb
Copy link
Contributor Author

nojb commented Jun 24, 2022

OK, pushed a new version:

nojebar@PERVERSESHEAF:~/ocaml$ TERM=dumb runtime/ocamlrun ./ocaml -I stdlib
OCaml version 5.1.0+dev1-2022-06-09
Enter #help;; for help.

# s.[i] <- c;;
Line 1, characters 0-10:
1 | s.[i] <- c;;
    ^^^^^^^^^^
Error: Syntax error: strings are immutable, there is no assignment syntax for them.
Hint: Mutable sequences of bytes are available in the Bytes module.
Hint: Did you mean to use 'Bytes.set'?

@gasche
Copy link
Member

gasche commented Jun 24, 2022

I'm okay with the proposed final version.

@nojb nojb merged commit bb03e11 into ocaml:trunk Jun 24, 2022
@nojb nojb deleted the remove_infix_set_op branch June 24, 2022 13:05
nojb added a commit that referenced this pull request Jun 24, 2022
(cherry picked from commit bb03e11)
@nojb
Copy link
Contributor Author

nojb commented Jun 24, 2022

Cherry-picked to 5.0: 67863ba

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

Successfully merging this pull request may close these issues.

"str.[i] <- c" is desugared into "String.set str i c" which does not exist anymore
6 participants