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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 0. in lexer #10025

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix 0. in lexer #10025

wants to merge 1 commit into from

Conversation

MrQubo
Copy link

@MrQubo MrQubo commented Feb 17, 2024

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 馃憤 to pull requests you find important.

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

@pennae
Copy link
Contributor

pennae commented Feb 17, 2024

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.

@MrQubo
Copy link
Author

MrQubo commented Feb 17, 2024

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.

@roberth
Copy link
Member

roberth commented Feb 23, 2024

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

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

@roberth
Copy link
Member

roberth commented Feb 23, 2024

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

@nixos-discourse
Copy link

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

@pennae
Copy link
Contributor

pennae commented Feb 29, 2024

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

@roberth
Copy link
Member

roberth commented Feb 29, 2024

@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.

@pennae
Copy link
Contributor

pennae commented Feb 29, 2024

@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.

@roberth
Copy link
Member

roberth commented Feb 29, 2024

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.

@thufschmitt
Copy link
Member

@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 #2911

@pennae
Copy link
Contributor

pennae commented Feb 29, 2024

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

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.

None yet

5 participants