rtphubeny commented 1 year ago

The existing implementation of substitution resolution inconsistently resolves transitive substitutions of Object contents because the Object is an unordered map. If a dependent is evaluated before the dependency, the dependent is not properly resolved.

For example:

{
  a: 5,
  b: ${a},
  c: ${b},
}

c will correctly resolve to 5 if b has been resolved first. However, if c is resolved first, c is evaluated to ${a}.

According to HOCON docs,

In general, in resolving a substitution the implementation must:

lazy-evaluate the substitution target so there's no "circularity by side effect"

"look forward" and use the final value for the path specified in the substitution

if a cycle results, the implementation must "look back" in the merge stack to try to resolve the cycle

if neither lazy evaluation nor "looking only backward" resolves a cycle, the substitution is missing which is an error unless the ${?foo} optional-substitution syntax was used.

This pull request does not specifically implement a "forward" or "backward looking" solution, but it does evaluate the graph for cycles and improves substitution resolution consistency.

codecov[bot] commented 1 year ago

Codecov Report

Base: 95.34% // Head: 95.39% // Increases project coverage by +0.05% :tada:

Coverage data is based on head (e6dc29c) compared to base (67c3b38). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #37 +/- ## ========================================== + Coverage 95.34% 95.39% +0.05% ========================================== Files 3 3 Lines 773 782 +9 ========================================== + Hits 737 746 +9 Misses 24 24 Partials 12 12 ``` | [Impacted Files](https://codecov.io/gh/gurkankaymak/hocon/pull/37?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=G%C3%BCrkan+Kaymak) | Coverage Δ | | |---|---|---| | [parser.go](https://codecov.io/gh/gurkankaymak/hocon/pull/37?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=G%C3%BCrkan+Kaymak#diff-cGFyc2VyLmdv) | `95.88% <100.00%> (+0.07%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=G%C3%BCrkan+Kaymak). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=G%C3%BCrkan+Kaymak)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

rebecca-hubeny-okcupid commented 1 year ago

@gurkankaymak can you please take a look at this pull request when you have the time?

gurkankaymak / hocon

Consistently resolve transitive substitutions #37

Codecov Report