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

regression (0.19.6 => 0.20): is broken with generic types #13066

Closed
timotheecour opened this issue Jan 7, 2020 · 7 comments · Fixed by #13303
Closed

regression (0.19.6 => 0.20): is broken with generic types #13066

timotheecour opened this issue Jan 7, 2020 · 7 comments · Fixed by #13303

Comments

@timotheecour
Copy link
Member

timotheecour commented Jan 7, 2020

Example

  type Foo[T1,T2] = object
  type Foo2 = Foo
  doAssert Foo2 is Foo # fails since 0.20
  doAssert Foo is Foo2 # fails since 0.20
  doAssert Foo is Foo # fails since 0.20

# EDIT
  doAssert Foo[int,float] is Foo2[int,float] # fails but also failed in 0.19.6

Current Output

`Foo2 is Foo`  [AssertionError]

Expected Output

works

Additional Information

@timotheecour
Copy link
Member Author

timotheecour commented Jan 8, 2020

after doing git bisect the culprit is this PR /cc @LemonBoy #8723

4808ef72dbfb44634a1afe4d88e657abe17cbfd5 is the first bad commit
commit 4808ef72dbfb44634a1afe4d88e657abe17cbfd5
Author: LemonBoy <LemonBoy@users.noreply.github.com>
Date:   Sun Oct 14 08:53:41 2018 +0200

    [WIP] Early evaluation of mIs (#8723)

    * Early evaluation of mIs

    The `evalIs` implementation was just a broken copy of `isOpImpl` so
    let's just avoid it alltogether: `mIs` nodes are either resolved during
    the semantic phase or bust.

    * Remove dead code and tidy it up

 compiler/semexprs.nim  |  6 ++----
 compiler/semfold.nim   | 43 +++----------------------------------------
 tests/magics/t8693.nim |  6 ++++++
 tests/types/tisopr.nim |  6 +++---
 4 files changed, 14 insertions(+), 47 deletions(-)

note:

git bisect is awesome; but it really works best if we keep using git rebase and not git merge as otherwise you get Bisecting: a merge base must be tested and it makes the process more complex / time consuming; see also #8664)

note:

the whole git bisect process could be further automated (as in D's dustmite https://github.com/CyberShadow/DustMite/wiki) if #11457 were implemented; right now each bisect step still may require manual intervention because there is no deterministic way to build old versions of nim

try it if you don't believe me:

git bisect bad v0.20.0
git bisect good v0.19.6

@timotheecour timotheecour changed the title regression (0.19.6 => 0.20): generic type alias broken regression (0.19.6 => 0.20): is broken with generic types Jan 8, 2020
@narimiran
Copy link
Member

the whole git bisect process could be further automated if #11457 [Lock down csources version] were implemented

So, like it currently is?

From https://github.com/nim-lang/csources:

This repository has been archived by the owner. It is now read-only.
This repository is archived because it's frozen, HEAD of csources can build Nim version 1 and any later version.

@timotheecour
Copy link
Member Author

timotheecour commented Jan 8, 2020

but that frozen csources repo may not be able to build future nim versions because of bootstrapping needs (say, if nim wants to rely on newly introduced features); see also #12917 which is related.

That gives me another idea though:

  • a nimble pkg (or stdlib module) that can build nim at any commit point in git history, including the past (say up till 0.18 since we may still have regressions that were introduced that far back). It works as follows:

  • pre-compute just once on a given machine: for each git tag in csources, checkout that tag and build build_nim_csources (see build_all.sh), saving to tag specific filename, eg:

bin/nim_csources_v0.18
bin/nim_csources_v0.19
bin/nim_csources_v0.20

to build nim at a given nim repo commit tag: simply try all the nim_csources in descending order until the 1st successful build.
If nothing succeeds, it's likely this git commit was a bad revision (eg because the committer didn't do squash and rebase and an intermediate bad state was checked in)

note

  • we can optimize by assuming older versions require older nim_csources, so that if bin/nim_csources_v0.19 was the 1st successful csources to build a nim git commit, then old nim git commits should start trying at bin/nim_csources_v0.19 and below

  • my assumption was that simply using choosenim (with appropriate version) would be enough, however that was note the case: for many git commits, a nim_csources worked, but none of the versions in choosenim worked.

  • the 1st part (building all tagged nim_csources) should probably be handled by choosenim as a feature request (just filed feature: choosenim csources v0.19.0 (for bootstrapping older nims during bisect) dom96/choosenim#154)

  • once we have ability to build any nim revision, it's trivial to integrate into a fully automated git bisect pipeline for easy bug pre-processing to identify which commit introduced a bug

@Araq
Copy link
Member

Araq commented Jan 9, 2020

Seems quite some work just to support a workflow ("git bisect") that is inherently flawed to begin with.

@timotheecour
Copy link
Member Author

actually I found a simpler way that simplifies a lot the above procedure:

  • we can ask git for which release should be used instead of having to try multiple ones:
    for a given commit (eg: 9985706)
git describe --abbrev=0
v0.19.0
nim c -o:bin/nim_temp1 -d:release --skipParentCfg --skipUserCfg --lib:lib compiler/nim.nim

and then we wrap all this logic in a nim function that also calls git bisect. Quite simple.

a workflow ("git bisect") that is inherently flawed to begin with.

not sure I follow; git bisect is widely used and I've used it many times (including here) to find the 1st commit introducing a bug

@Araq
Copy link
Member

Araq commented Jan 10, 2020

It's flawed because it has false positives.

@timotheecour
Copy link
Member Author

It's flawed because it has false positives.

not sure what you mean by that, I've been using it very reliably a number of times, it's a standard tool and it works.
Note you can use special exit code 125 to skip commits that are broken (eg make || exit 125) inside your git bisect run command

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants