by

Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • 17:54
    loredanacirstea commented #9622
  • 17:48
    axic commented #9622
  • 17:42
    loredanacirstea commented #9622
  • 17:23
    axic commented #9622
  • 17:10
    loredanacirstea commented #9622
  • 16:49
    ekpyron commented #9330
  • 16:46
    ekpyron commented #9330
  • 16:45
    axic commented #9330
  • 16:45
    ekpyron commented #9330
  • 16:44
    axic commented #9622
  • 16:43
    loredanacirstea commented #9622
  • 16:42
    bshastry labeled #9623
  • 16:42
    bshastry opened #9623
  • 16:42
    ekpyron commented #9330
  • 16:39
    loredanacirstea opened #9622
  • 16:16
    axic synchronize #9331
  • 16:16

    axic on yul-token

    Use only advance() and not with… Add scanner test for specially … Do not emit Solidity-only token… and 2 more (compare)

  • 16:15
    axic synchronize #9549
  • 16:15

    axic on yul-reserved

    Add reservedIdentifier to diale… Disallow using reserved identif… Disallow using reserved identif… and 4 more (compare)

  • 16:09

    axic on yul-tests

    (compare)

Daniel Kirchner
@ekpyron
@axic By the way: why not just demote ethereum/solidity#9549 to warnings for 0.7 and stage making them errors for 0.8? Then it'd not be breaking...
Alex Beregszaszi
@axic
warning in the analyser for the user facing code, but disallowing it in the optimiser part, right?
Daniel Kirchner
@ekpyron
Yep, I'd approve that right away.
Alex Beregszaszi
@axic
does the sol-yul allows warnings pass through?
or it will freak out somwhere
Daniel Kirchner
@ekpyron
Not sure... but breaking sol->yul would be fine, that's experimental anyways... only in regular old-codegen inline assembly we shouldn't just ignore it being breaking.
Alex Beregszaszi
@axic
The bigger question is https://github.com/ethereum/solidity/pull/9549/files#r462495913 and that goes into the realm of requiring #9620
Daniel Kirchner
@ekpyron
If you make it warnings, we can just warn about them everywhere, problem solved, isn't it :-)?
It's not like it should be fine outside the object dialect - it's just that we can't just start breaking the non-object dialect, because it's used in inline assembly... but warnings don't break anything :-).
Alex Beregszaszi
@axic

But should we disallow those in the first place? Probably yes.

Maybe even warning is the best option long term, because users could use those function names if they are keen on it.

Daniel Kirchner
@ekpyron
We can still decide if warnings are enough (I would say so, but would be open to promoting to errors) or not when moving to 0.8.
And warning about them seems reasonable in general.
On the other hand using the actual jump or dup* opcodes in inline assembly has been disallowed since 0.5.0, so I'm also not sure we have to do anything about them ever again...
It's actually rather weird to have them be opcodes in 0.4, then disallow them as opcodes and as identifiers in 0.5, then allow them again as identifiers in 0.6 (a.k.a. they are gone for long enough, s.t. we can think of them as never having existed) and then suddenly start warning about them again in 0.7...
That's why I've always been rather sceptical about this... but a warning I could still live with.
Alex Beregszaszi
@axic
Because someone made a mistake in 0.6.0
Introducing the dialects forgot to keep that information about reserved opcodes
Daniel Kirchner
@ekpyron
I think it was a justified and maybe even conscious decision.
There is no such thing as reserved opcodes anymore since 0.5 :-D.
Alex Beregszaszi
@axic
Based on git history it seems like an oversight
Daniel Kirchner
@ekpyron
At least to me it seems more then fine to have 0.6 just treat them normally as identifiers, after they were disallowed with verbose error message for the entire lifetime of 0.5.
Alex Beregszaszi
@axic
I’d say for the “inlineassembly” dialect they should be disallowed. For pure yul I could understand being lenient.
I’d even suggest creating a specific dialect for inline assembly :)
Daniel Kirchner
@ekpyron
The only reason for disallowing them in inline assembly is that they were allowed there years ago - but what's the point in having breaking releases, if we can't just drop such old baggage several major releases later :-)?
Alex Beregszaszi
@axic
No, the reason we are disallowing it in inline assembly to reduce confusion.
Because malicious code could make it look like they are EVM opcodes.
Daniel Kirchner
@ekpyron
I think that's a rather artificial concern. But yes, following that argument I'd be fine with making it a warning :-).
Alex Beregszaszi
@axic
Imagine we add support for calling sol functions from yul blocks (there is an issue for this). Then someone could have a solidity functions called jump and do something weird trying to hide it from auditors.
Daniel Kirchner
@ekpyron
But what? The only arguments it could get would be variables, which can never be used to point to functions - any auditor who could be tricked by that should probably be fired :-). But yeah, still: I do see the point, I just don't think it's a strong one :-).
But yeah - the discussion is rather mute - we can just make it a warning and revisit for 0.8.
Alex Beregszaszi
@axic
There is a design issue for this and it was discussed on a call. You can comment and bring it up for the next call. I’m fine if people vote against it.
Daniel Kirchner
@ekpyron
I cannot really imagine that discussion actually :-).
Alex Beregszaszi
@axic
I am more interested in getting the parser refactor/improvement things merged than this restriction pr tbh
Daniel Kirchner
@ekpyron
Fair enough :-).
How the hell did you find this in the git history by the way :-)?
Alex Beregszaszi
@axic
Via the cleanup PRs I did already in this regard. Those were merged a few weeks ago
Alex Beregszaszi
@axic
I dont care about that PR as much as about the refactor + keyword stuff :)
Daniel Kirchner
@ekpyron
I was just curious... but yeah, I just found it - it was part of a 20 commit PR fixing an issue with zero comments or discussion in it... so you're probably right and I'm misremembering :-).
Leonardo
@leonardoalt
@ekpyron it'd be good to know why it's suddenly happening though...
Daniel Kirchner
@ekpyron
Well, we could compare the run time of the job with older runs... but my best guess, if nothing else changed, is just random CircleCI compute power fluctuations...
Daniel Kirchner
@ekpyron
@axic By the way: I'm not sure about the grammar and https://github.com/solidity-parser/parser ... they, of course, rely on the categories defined in the old grammar to build there AST here: https://github.com/solidity-parser/parser/blob/master/src/ASTBuilder.js - so that would need to be adjusted to work with any new grammar. I guess it won't work against the one in our repo either, so (I'll try in a second)... which is probably why they still point to https://github.com/solidity-parser/antlr/tree/e6fe8be1c76759f7ea72da3518d8531e8894d28d for the grammar... so at least to me it seems that the way to deal with that is rather for us to do whatever we want to the grammar, and then them deciding whether they want to stick to the old or refactor for the new one... but I'm not entirely sure...
Yeah, half of their tests already fail with the grammar currently in our repo...
Alex Beregszaszi
@axic
I’m leaning towards the opinion that the grammar we have should be useful as a specification (and that works extremely well with this “train diagram”). Having that legacy that a version of the grammar was used by an external project and now they are not responsive to collaborate should not block us from doing anything.
Daniel Kirchner
@ekpyron
:+1: I agree, just making sure we're on the same page about that.
Alex Beregszaszi
@axic
We two are, but not sure rest of the team shares it lol
Daniel Kirchner
@ekpyron
Well, the current grammar is blocking us and we didn't have much responsiveness about it so far, so it's basically not very different :-D.
And as I said: the grammar in our repo is already incompatible with https://github.com/solidity-parser/parser anyhow, so...
In case you're curious: that's by the way the set of features the old grammar was supposed to cover: https://github.com/solidity-parser/parser/blob/master/test/test.sol
Alex Beregszaszi
@axic
I seen that test file, but haven’t explored it too much
Alex Beregszaszi
@axic
@ekpyron #9622