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

POC - Unicode chars in identifiers #317

Merged
merged 5 commits into from
Dec 20, 2024

Conversation

nmigueles
Copy link
Contributor

Suuuper POC, seguro le falta algo, pero por curiosidad despues de una charla con @fdodino este miercoles, que le paso a un compañero que puso una ñ en el codigo y le fallaba de formas extravagantes, surgio que algunos lenguajes como golang y otros (sabia de otro pero se me fue ahora) permiten en la definición del lenguaje no solo ascii si no unicode.

Def de golang https://go.dev/ref/spec#raw_string_lit que hace referencia a como usan unicode (aca la parte de string literals).

Peeero bueno, como dije por curiosidad y por falta de tiempo solo hice un mini POC para soportar unicode en los identificadores o mejor dicho en el mini parser de name .

Lo probe con un par de tests pero son a modo de ilustración. Me gustaria probarlo a manopla a ver si encuentro algun caso raro.

Me gusto como experimiento y no tengo tanto conocimiento del parser como para ver si hay que tocar algo más.

@nmigueles nmigueles self-assigned this Nov 24, 2024
@@ -194,7 +194,7 @@ export const Import: Parser<ImportNode> = node(ImportNode)(() =>
// COMMON
// ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────

export const name: Parser<Name> = lazy('identifier', () => regex(/[^\W\d]\w*/))
export const name: Parser<Name> = lazy('identifier', () => regex(/^[\p{L}_][\p{L}\p{N}_]*/u))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh aqui la magic, ese \p{L} hace referencia a cualquier unicode letter, que contiene a todos los \w (menos al _ creo)

@@ -1871,6 +1878,10 @@ class c {}`
'var v'.should.be.parsedBy(parser).into(new Variable({ name: 'v', isConstant: false })).and.be.tracedTo(0, 5)
})

it('should parse var declaration with non-ascii caracter in identifier', () => {
'var ñ'.should.be.parsedBy(parser).into(new Variable({ name: 'ñ', isConstant: false })).and.be.tracedTo(0, 5)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🔥 para poder escribir codigo 100% en castellano

Copy link
Contributor

Choose a reason for hiding this comment

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

uh qué bueno!!

quiero ver si pasa también por el tamiz del validador, el cli y el LSP-IDE, pero definitivamente lo quiero!! 👏🏼 👏🏼 👏🏼

@fdodino
Copy link
Contributor

fdodino commented Dec 13, 2024

@nmigueles avisanos si ésto lo podemos probar con @PalumboN e @ivojawer en alguna sesión de los martes, a mí me gustaría que en 2025 entremos al siglo XXII y podamos definir

var rufián = "El Maloso"
var ñoño = "Mirelo a él"

@nmigueles
Copy link
Contributor Author

Pero ma vale! @fdodino yo lo probé un poquito y no pinchaba nada inmediato, pero hay que revisar de punta a punta.

@fdodino fdodino marked this pull request as ready for review December 16, 2024 00:33
@fdodino
Copy link
Contributor

fdodino commented Dec 16, 2024

@nmigueles , me lo bajé localmente, y cuando hice este wlk

object pajarito {
  var property ñoño = true
  
  method superó() = !ñoño
  method seráÑoño() = ñoño
}

me tiraba este error:

dodain@local  $       wollok-ts-cli   master  npm run start -- repl /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita/bug302.wlk --skipValidations -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita 

> [email protected] start
> node ./build/src/index.js repl /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita/bug302.wlk --skipValidations -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita

🖥️  Initializing Wollok REPL for file /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita/bug302.wlk on /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita
💥 Uh-oh... Unexpected Error!
Failed to parse bug302.wlk: 
-- PARSING FAILED --------------------------------------------------

  4 |   method superó() = !ñoño
  5 |   method seráÑoño() = ñoño
> 6 | }
    | ^

Expected one of the following: 

'@', 'class', 'const', 'describe', 'mixin', 'object', 'only', 'package', 'program', 'test', 'var', '{', EOF, comment, not "}", whitespace

claro, porque estaba en tu master. Me fui a la branch POC-unicode-identifiers y ahora sí:

npm run start -- repl /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita/bug302.wlk --skipValidations -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita 

> [email protected] start
> node ./build/src/index.js repl /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita/bug302.wlk --skipValidations -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita

🖥️  Initializing Wollok REPL for file /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita/bug302.wlk on /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita
wollok:bug302> ✓ Dynamic diagram available at: http://localhost:3000
wollok:bug302> pajarito.ñoño()
✓ true
wollok:bug302> pajarito.seráÑoño()
✓ true
wollok:bug302> pajarito.superó()
✓ false

Hay que arreglar el validador porque dice que ñoño debería comenzar con minúscula (el regex que usamos no detecta caracteres Unicode).

Creé un test:

import bug.*

test "ñoño is what ñoño does, dice mi mamá" {
  assert.that(pajarito.seráÑoño())
}

y anduvo ok

npm run start -- test -f 'testBug.wtest' -t 'ñoño is what ñoño does, dice mi mamá'  -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita 

> [email protected] start
> node ./build/src/index.js test -f testBug.wtest -t ñoño is what ñoño does, dice mi mamá -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita

🧪 Running all tests matching 'testBug.wtest'.*.'ñoño is what ñoño does, dice mi mamá' on /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita
🌏 Building environment for /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita...

[WARNING]: nameShouldBeginWithLowercase at bug.wlk:2
Running 1 tests...
 testBug
   ✓ "ñoño is what ñoño does, dice mi mamá"


 ✓ 1 passing  

También un program:

import bug.*

program ñamfrifrufrifalifrú {
  console.println(pajarito.seráÑoño())
  console.println("se finí")
}

Y lo ejecuté

npm run start -- run bugcito.ñamfrifrufrifalifrú  -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita 

> [email protected] start
> node ./build/src/index.js run bugcito.ñamfrifrufrifalifrú -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita

🚀 Running program bugcito.ñamfrifrufrifalifrú as a program on /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita
🌏 Building environment for bugcito.ñamfrifrufrifalifrú...

[WARNING]: nameShouldBeginWithLowercase at bug.wlk:2
true
se finí

voy a seguir probando

@fdodino
Copy link
Contributor

fdodino commented Dec 16, 2024

El toque que hay que hacerle al validador es éste:

export const nameShouldBeginWithUppercase = nameMatches(/^[A-ZÑÁÉÍÓÚ]/)

export const nameShouldBeginWithLowercase = nameMatches(/^[a-z_<ñáéíóú]/)

para incluir nuestros caracteres especiales, lo siento si algún austríaco quiere ponerle a una clase Austria en alemán, que sería Österreich, o quiere ponerle über a una variable (que sería arriba), no conozco palabras que comiencen con acentro grave en francés.

Con éso ya me anduvo joya ejecutar los tests y el programa sin errores de validación:

dodain@local  $       wollok-ts-cli   master  npm run start -- run bugcito.ñamfrifrufrifalifrú  -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita 

> [email protected] start
> node ./build/src/index.js run bugcito.ñamfrifrufrifalifrú -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita

🚀 Running program bugcito.ñamfrifrufrifalifrú as a program on /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita
🌏 Building environment for bugcito.ñamfrifrufrifalifrú...

✓ No problems found building the environment!
true
se finí
dodain@local  $       wollok-ts-cli   master  npm run start -- test -f 'testBug.wtest' -t 'ñoño is what ñoño does, dice mi mamá'  -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita 

> [email protected] start
> node ./build/src/index.js test -f testBug.wtest -t ñoño is what ñoño does, dice mi mamá -p /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita

🧪 Running all tests matching 'testBug.wtest'.*.'ñoño is what ñoño does, dice mi mamá' on /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita
🌏 Building environment for /home/dodain/workspace/wollok-dev/wollok-workspace/pruebita...

✓ No problems found building the environment!
Running 1 tests...
 testBug
   ✓ "ñoño is what ñoño does, dice mi mamá"


 ✓ 1 passing  

@fdodino
Copy link
Contributor

fdodino commented Dec 16, 2024

Apa! Pude mergear!

image

ampliaremos

@fdodino
Copy link
Contributor

fdodino commented Dec 16, 2024

bueno, acá tienen una demo haciendo un pkg de wollok-ts-cli:

coneñe

acá se ve que

  • funciona ok el parser dentro de LSP-IDE
  • el validador no salta por el falso lowercase de las variables
  • ts-cli se integra bien para repl, tests y program (lo que me preocupaba era correrlo con el sistema operativo, habría que ver en Windows @nmigueles si vos lo tenés)
  • no hace falta toquetear nada del diagrama dinámico
  • y el autocompletado?

image

image

  • el formatter también lo probé
  • yo creo que lo único que podríamos hacer es abrir otro PR para language para incorporar estos caracteres en los sanity tests, pero ésto ya estaría para mergear @PalumboN @ivojawer

Copy link
Contributor

@fdodino fdodino left a comment

Choose a reason for hiding this comment

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

probado y aprobado!

@nmigueles
Copy link
Contributor Author

export const nameShouldBeginWithUppercase = nameMatches(/^[A-ZÑÁÉÍÓÚ]/)

export const nameShouldBeginWithLowercase = nameMatches(/^[a-z_<ñáéíóú]/)

@fdodino vengo con una propuesta superadora para el contento de los Alemanes

export const nameShouldBeginWithUppercase = nameMatches(/^\p{Lu}/u);

export const nameShouldBeginWithLowercase = nameMatches(/^\p{Ll}/u);

@fdodino
Copy link
Contributor

fdodino commented Dec 17, 2024

export const nameShouldBeginWithUppercase = nameMatches(/^[A-ZÑÁÉÍÓÚ]/)

export const nameShouldBeginWithLowercase = nameMatches(/^[a-z_<ñáéíóú]/)

@fdodino vengo con una propuesta superadora para el contento de los Alemanes

export const nameShouldBeginWithUppercase = nameMatches(/^\p{Lu}/u);

export const nameShouldBeginWithLowercase = nameMatches(/^\p{Ll}/u);

aah, sí, lo vi pero no estaba 100% seguro, sí, vamos por ésa, podés probarlo agregando un sanity test en Language.

@fdodino
Copy link
Contributor

fdodino commented Dec 19, 2024

@nmigueles , updates de lo que estuve probando:

Agregados

Generé un PR en language que me gustaría que pasen para confirmar que todo está ok.

Cosas del PR

  1. Estos cambios que sugeriste
export const nameShouldBeginWithUppercase = nameMatches(/^\p{Lu}/u);
export const nameShouldBeginWithLowercase = nameMatches(/^\p{Ll}/u);

no funcionaron al correr los tests del validador, rompieron de hecho 24 tests. Podés darle una vuelta de tuerca si querés pero me gustaría que lo chequees contra la branch nueva de language para confirmar que pasan.

  1. Me gustaría confirmar que los strings se banquen caracteres unicode, ya que estamos haciendo este cambio que sea con entrada triunfal.

  2. El sanity test me permitió ver que hay un error en el intérprete, cuando querés hacer new Niño() se rompe

wollok.lang.EvaluationError: Error: Could not resolve reference to new
        at basics.unicode."pingüinos y ñandúes"."serán amados por Jorge" [basics/unicode.wtest:22]
     Derived from TypeScript stack
      at Evaluation.exec (src/interpreter/runtimeModel.ts:438:33)
      at exec.next (<anonymous>)
      at Evaluation.execVariable (src/interpreter/runtimeModel.ts:497:31)

hay que mirarlo un toque más con @PalumboN e @ivojawer si querés pero me preocupa y por eso paso el PR a requested changes.

El resto está 100 puntos.

@fdodino fdodino self-requested a review December 19, 2024 02:57
Copy link
Contributor

@fdodino fdodino left a comment

Choose a reason for hiding this comment

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

Lo paso a requested changes por lo que puse arriba, no me peguen, soy Dodain.

@fdodino
Copy link
Contributor

fdodino commented Dec 20, 2024

Lo paso a requested changes por lo que puse arriba, no me peguen, soy Dodain.

Bueno, ahí le encontré la vuelta, pasan los test del PR de language que armé, así que sale con fritas @PalumboN !

Copy link
Contributor

@fdodino fdodino left a comment

Choose a reason for hiding this comment

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

🍾

test/parser.test.ts Show resolved Hide resolved
@fdodino fdodino merged commit 59c43f7 into uqbar-project:master Dec 20, 2024
1 check passed
npasserini pushed a commit that referenced this pull request Dec 29, 2024
* poc

* Fix validator

* Fix for new reference

---------

Co-authored-by: Fernando Dodino <[email protected]>
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.

2 participants