nix icon indicating copy to clipboard operation
nix copied to clipboard

Fix 0. in lexer

Open MrQubo opened this issue 1 year ago • 4 comments

Motivation

0. is not parsed correctly as a float.

Context

Currently, 0. is tokenized as INT '.'.

This change will also allow 01.123, but INT token is defined as just [0-9]+ so this will make parser more consistent. Currently, 01 is parsed as INT, but 01.0, gives an error attempt to call something which is not a function but an integer.

Priorities and Process

Add :+1: to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

MrQubo avatar Feb 17 '24 00:02 MrQubo

this is a language-breaking change and cannot be done just like this. if we ever get a nixlang revision a la rfc137 this should absolutely be part of the syntax revision, but current nixlang cannot do this without breaking stuff.

pennae avatar Feb 17 '24 01:02 pennae

You're right, this changes how this example expression is parsed: [ 01.1 ]. Previously it was [ 1 0.1 ], with this patch it's [ 1.1 ].

Here's another example: 0.a or 2. Previously the result was 2, after the patch it results in an error.

MrQubo avatar Feb 17 '24 02:02 MrQubo

Here's another instance of the "list item separator" making things difficult.

  • https://github.com/NixOS/nix/issues/7578#issuecomment-1406539912

Without versioning the language, which is also not exactly great, the best thing we can do is to warn about such "ambiguous" syntax.

roberth avatar Feb 23 '24 10:02 roberth

Best not to work on the parser until pennae's rewrite is merged

  • #9989

roberth avatar Feb 23 '24 10:02 roberth

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-28-nix-team-meeting-129/40499/1

nixos-discourse avatar Feb 29 '24 07:02 nixos-discourse

since this was listed as "blocked on parser rewrite" in the meeting notes but #9971 was rejected outright, we will add here:

this should also be rejected outright for the exact same reason

pennae avatar Feb 29 '24 14:02 pennae

@pennae Does that mean you disagree with the idea of keeping the same behavior, while adding a warning about this suspicious syntax?

we will add here

For those who don't already know, usually we refers to their personal pronoun, which can't be distinguished from the pronoun for a group. I respect that and I believe it's important that others know this to, for respectful, fair and effective communication.

roberth avatar Feb 29 '24 15:02 roberth

@pennae Does that mean you disagree with the idea of keeping the same behavior, while adding a warning about this suspicious syntax?

without infrastructure that would also allow #9971 to emit a warning for suspicious indentation? yes. this kind of issue should be handled consistently across all instances.

pennae avatar Feb 29 '24 15:02 pennae

Of course we need the right infrastructure for both cases to work. I can't estimate the feasibility or cost of this, and I don't want to burden you with these issues any more than necessary. Let's take one step at a time, the step being the rewrite. We can worry about these warnings later.

roberth avatar Feb 29 '24 15:02 roberth

@pennae : just to make it clear (because that might indeed not have been obvious from the notes about #9971): we said that the semantic change was a no-go, but a warning was explicitly suggested in https://github.com/NixOS/nix/pull/2911

thufschmitt avatar Feb 29 '24 20:02 thufschmitt

@thufschmitt ok, the we're on the same page after all :) very good

pennae avatar Feb 29 '24 20:02 pennae