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

Fix #412: Protect the state of packages and loaders from exceptions. #416

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

sjrd
Copy link
Contributor

@sjrd sjrd commented Dec 4, 2023

At the loader level:
Only remove package data entries after successful loading.

At the package symbol level:
While loading new symbols from the loader, all new declarations are put in a pendingDeclarations safe zone. They are committed to myDeclarations only after the whole operation successfully complets.

This way, if a failure happens while reading one root, it does not corrupt the other roots in the same package.


Locally tested with

    libraryDependencies += "dev.zio" %% "zio-test" % "2.0.16",

and the following TypeSuite test:

  testWithContext("issue-412") {
    val zioPackage = ctx.findPackage("zio")

    // Cause the initial failure
    intercept[TastyFormatException](zioPackage.declarations)

    val decl1 = zioPackage.findDecl(typeName("ZInputStream")) // before problematic file
    assert(decl1.isClass)

    val decl2 = zioPackage.findDecl(typeName("ZOutputStream")) // after problematic file
    assert(decl2.isClass)

    val decl3 = zioPackage.findDecl(termName("package")).asTerm // original issue
    assert(decl3.isModuleVal)

    // Trying to read the problematic file still reports the same exception
    intercept[TastyFormatException](zioPackage.findDecl(termName("ZLayerCompanionVersionSpecific")))

    // Trying to load everything again still reports the same exception
    intercept[TastyFormatException](zioPackage.declarations)
  }

@sjrd sjrd requested a review from adpi2 December 4, 2023 16:57
@sjrd sjrd marked this pull request as draft December 4, 2023 17:31
@sjrd
Copy link
Contributor Author

sjrd commented Dec 4, 2023

Hum this is still broken. I'll fix it tomorrow.

…xceptions.

At the loader level:
Only remove package data entries *after* successful loading.

At the package symbol level:
While loading new symbols from the loader, all new declarations
are put in a `pendingDeclarations` safe zone. They are committed
to `myDeclarations` only after the whole operation successfully
complets.

This way, if a failure happens while reading one root, it does not
corrupt the other roots in the same package.
@sjrd sjrd force-pushed the fix-inter-files-corruption branch from 1de2317 to 692aa59 Compare December 5, 2023 08:08
@sjrd sjrd marked this pull request as ready for review December 5, 2023 08:15
@sjrd
Copy link
Contributor Author

sjrd commented Dec 5, 2023

OK I believe this really good, now. I enhanced the test in the PR description.

@sjrd sjrd merged commit c1f1ba7 into scalacenter:main Dec 6, 2023
4 checks passed
@sjrd sjrd deleted the fix-inter-files-corruption branch December 6, 2023 08:34
@adpi2
Copy link
Member

adpi2 commented Dec 6, 2023

Actually there is a little regression in this. Before it would add all the symbols in the file up to the exception, but now it does not add any. Is this the desired behavior?

object Foo

class Foo

I cannot find the object Foo anymore even if the exception is in the TASTy part of class Foo.

@sjrd
Copy link
Contributor Author

sjrd commented Dec 6, 2023

Yes, that's expected. A companion and its class may have symbolic links between each other. If we throw away one but keep the other, it may contain and expose references to Symbols that were never properly completed.

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