Accept negative start line number #1783

Activity

1. eregon

   added

   needed for correct compilationNeeded to have correct compilation using PrismNeeded to have correct compilation using Prism

   on Nov 7, 2023
2. enebo commented on Nov 7, 2023

   

   enebo

   on Nov 7, 2023

   Collaborator

   More actions

   👍

           // FIXME: I believe line number can be negative?
           // Line Number
           appendUnsignedInt(metadata, lineNumber);
   
3. enebo commented on Nov 7, 2023

   

   enebo

   on Nov 7, 2023

   Collaborator

   More actions

   @eregon I don't understand your label? JRuby certainly will fail a test or small number of tests because of this behavior but why would this be a blocker? Is any bug which is not green a blocker over the old parser? Is there really a difference here from bug or is this a way of signalling priority?
4. eregon commented on Nov 7, 2023

   

   eregon

   on Nov 7, 2023

   MemberAuthor

   More actions

   I splitted the previous label in two because it wasn't clear.
   It's intended to signal priority yes.
   The meaning is that this issue is blocking correct compilation (of such eval's) and blocking truffleruby's adoption of prism (which is just another case of compilation, so maybe I should remove the /truffleruby suffix).
5. eregon

   changed the title [-]Accept negaitve start line number[/-] [+]Accept negative start line number[/+] on Nov 7, 2023
6. enebo commented on Nov 7, 2023

   

   enebo

   on Nov 7, 2023

   Last edited by enebo

   Collaborator

   More actions

   @eregon I have been not reporting a lot of smaller issues to prevent overwhelming the issue system and diluting focus. Once you can run ruby/spec or mri tests there are probably hundreds of issues I can open. Many are things like exception text mismatches or missing errors/warns but compared to reported issues like not properly encoding strings or regexps this feels like not a priority.

   That paragraph may have implied you should not have even opened the issue. I don't mean that. I just mean from a priority perspective this feels not too important other than passing a test suite. It sounds like some admonishment or criticism but I was really confused by the word "blocker".
7. eregon commented on Nov 7, 2023

   

   eregon

   on Nov 7, 2023

   MemberAuthor

   More actions

   Yeah blocker was probably too strong without more context, I renamed to needed for correct compilation.
   I'm trying to identify issues which do prevent switching from the current parser to Prism in TruffleRuby (by default, and removing the current parser).
   I annotated the issues that I consider are blocking that, because it would be too incorrect or messy to work around (and since we are designing a new parser we don't want to make things difficult to use).
   This specific issue I think is hard to work around so I think it qualifies for that (and it's likely easy to fix).

   OTOH things like different warnings messages or missing SyntaxError I think are not very important for that switch, those could be fixed later from the POV of using Prism in TruffleRuby.
8. enebo commented on Nov 7, 2023

   

   enebo

   on Nov 7, 2023

   Collaborator

   More actions

   Thanks for your perspective on it. It is clearer now.
9. kddnewton

   added this to the Ruby 3.3.0 milestone on Nov 9, 2023
10. enebo

    added a commit that references this issue on Nov 10, 2023

    Partial fix for ruby#1783 for Java Loader

    ...

    4329790
11. kddnewton

    added

    good first issueGood for newcomersGood for newcomers

    on Nov 17, 2023
12. casperisfine commented on Nov 29, 2023

    

    casperisfine

    on Nov 29, 2023

    Contributor

    More actions

    Possible fix: #1944
13. kddnewton

    closed this as completedin #1944on Nov 29, 2023