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

Text/replace on interpolated text #1084

Merged
merged 5 commits into from
Oct 25, 2020

Conversation

basile-henry
Copy link
Collaborator

@basile-henry basile-henry commented Oct 17, 2020

This change to the beta normalization rules ensures that the normalization stays consistent.

The current haskell implementation has this weird behaviour:

➜ dhall repl
Welcome to the Dhall v1.35.0 REPL! Type :help for more information.
⊢ :let f = \(x : Text) -> Text/replace "a" "b" "a ${x} a"

f : ∀(x : Text) → Text

⊢ f "a"

"b a b"

⊢ let g = \(x : Text) -> Text/replace "a" "b" "a ${x} a" in g "a"

"b b b"

As @Gabriel439 suggested in #1065 an alternative rule could be:

require the haystack to be non-abstract before further reduction can occur

That would definitely be simpler to implement, but maybe not as strongly normalising as it could be.
Is there anything else I need to change? Should this change be documented somehow?

EDIT:
As @Nadrieril pointed out, Gabriel's suggestion really is the only rule possible here. I changed this PR to implement this suggestion instead! 😄

This change to the beta normalization rules ensures that the normalization stays consistent.

The current haskell implementation currently has this weird behaviour:

```
➜ dhall repl
Welcome to the Dhall v1.35.0 REPL! Type :help for more information.
⊢ :let f = \(x : Text) -> Text/replace "a" "b" "a ${x} a"

f : ∀(x : Text) → Text

⊢ f "a"

"b a b"

⊢ let g = \(x : Text) -> Text/replace "a" "b" "a ${x} a" in g "a"

"b b b"
```
@Nadrieril
Copy link
Member

I believe that "require the haystack to be non-abstract before further reduction can occur" is the only sensible rule here. For example in \(x : Text) -> Text/replace "aa" "b" "a${x}", we don't know if replacement will occur without knowing x, nor can we proceed partially without complicated analysis of the overlap between the needle and what we know about the haystack.

@basile-henry
Copy link
Collaborator Author

Good point, I didn't think about this case. 😅 I will edit the changes to the spec to reflect this tomorrow!

@basile-henry
Copy link
Collaborator Author

There is one slightly different rule possible. If the needle is empty, then the haystack does not need to be fully evaluated.

So:

Text/replace ""

Could be normalized to:

\(_replacement : Text) \(haystack : Text) -> haystack

But that goes against "builtins are fully saturated". So maybe just this:

Text/replace "" replacement haystack

Is normalized to:

haystack

Regardless of whether haystack is in normal form or not.

Is this something we might want?

@philandstuff
Copy link
Collaborator

This special case feels like pretty low value, I’d rather stick with the general rule for all cases.

@Nadrieril
Copy link
Member

While you're at it, could you also add a test for \(x : Text) -> Text/replace "a" x "aaa" and λ(x : Text) → Text/replace "aa" "b" "a${x}"?

Copy link
Contributor

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this! 🙂

@Nadrieril
Copy link
Member

As per the rules, I believe you can now merge this, since there are no objections

@basile-henry
Copy link
Collaborator Author

Alright great! I've updated the branch. I don't think I have write access in this repo though, so someone else will have to merge this PR. 😄

@Nadrieril Nadrieril merged commit e1e726f into dhall-lang:master Oct 25, 2020
@basile-henry basile-henry deleted the text-replace branch October 25, 2020 15:32
@Gabriella439
Copy link
Contributor

@basile-henry: I just added you to the repository

@basile-henry
Copy link
Collaborator Author

Great, thanks! 😄

@Gabriella439
Copy link
Contributor

You're welcome! 🙂

philandstuff added a commit to philandstuff/dhall-golang that referenced this pull request Nov 4, 2020
Previously, `Text/replace "foo" "bar${x}baz" "foofoo"` did not work as
expected.  This fixes it by extracting the code from eval.go that
deals with evaluating Text into a textValBuilder struct, and then
reusing that textValBuilder in the Text/replace implementation.

This pins us to a dhall-lang commit just after
dhall-lang/dhall-lang#1084 was merged; this is
the same as verison 19.0.0 with a fix for
TestNormalization/unit/TextReplaceAbstractA.
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.

None yet

4 participants