Navigation Menu

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

struct-copy fails if the structure type transformer binding is renamed #1399

Closed
lexi-lambda opened this issue Jul 27, 2016 · 10 comments
Closed

Comments

@lexi-lambda
Copy link
Member

lexi-lambda commented Jul 27, 2016

The following example fails with a syntax error:

#lang racket/base

(module struct racket/base
  (provide (struct-out point))
  (struct point (x y) #:transparent))

(require (rename-in 'struct [point point2d]))

(struct-copy point2d (point2d 1 2) [x 3])
struct-copy: accessor name not associated with the given structure type
  in: point2d-x

This happens because struct-copy synthesizes accessor names by concatenating the symbol provided as the structure type with each field symbol, separated by a hyphen. This seems incorrect, given that it assigns significance to the identifier’s name when really its binding should be all that matters.

@LeifAndersen
Copy link
Member

heh, lol, who needs hygeen anyway... ^_^

@AlexKnauth
Copy link
Member

Oh, wow

@LeifAndersen
Copy link
Member

I think this can actually be solved by passing in a local-expand to the id used in struct. I'll give this a shot.

@LeifAndersen
Copy link
Member

Oh wait, wrong way....never mind. :/

@AlexKnauth
Copy link
Member

AlexKnauth commented Nov 4, 2016

No, this should be solved by using the struct-info to get the correct accessors. (I thought that's what it already did; apparently not (no, it does, but what is doesn't do is use the struct-info to get the correct struct name))

@lexi-lambda
Copy link
Member Author

It’s not possible to fix struct-copy to get the correct accessors (I would have done it myself if it were possible) because it specifically does not require the user to specify the whole struct field names. That is, it allows users to specify x instead of point-x or point2d-x in the original example. A naïve implementation could try to make guesses based on matching name suffixes, but it isn’t too hard to come up with pathological cases in the presence of struct subtyping.

The right fix for this is probably changing something much more fundamental about information exposed by struct, which is a lot trickier to do in a backwards-compatible way.

@AlexKnauth
Copy link
Member

It would need to go through all the accessors, searching for one which has that field at the end? That's not ideal. Does the struct info have actual field names in it anywhere, separate from the accessors?

@lexi-lambda
Copy link
Member Author

No, it does not. There’s not even an obvious way to add it, given that it’s possible to have duplicate field names in the presence of struct inheritance. This is totally legal:

(struct foo (a))
(struct bar foo (a))

The struct-copy macro gets around this with the #:parent option, but that’s a little bit hacky.

Personally, I think the best fix for this would be to introduce a way to do struct copying at a low level rather than having it be a derived concept, which would avoid the slicing problem that struct-copy has. Improving the static type information provided by struct would be helpful, too, but it wouldn’t solve all of the issues with struct-copy and subtyping.

@samth
Copy link
Sponsor Member

samth commented Nov 4, 2016

What @lexi-lambda said. struct-copy is hopeless and can't be fixed without major changes to how structs work.

@sorawee
Copy link
Collaborator

sorawee commented Jun 16, 2019

#:parent can also go wrong...

#lang racket

(module a racket
  (provide a)
  (struct a (b-c) #:transparent))

(require 'a)
(struct a-b a (c) #:transparent)

(struct-copy a-b (a-b 1 2) [b-c #:parent a 10])
(struct-copy a-b (a-b 1 2) [c 10])

Both outputs (a-b 1 10)

sorawee added a commit to sorawee/racket that referenced this issue Jun 16, 2019
This PR fixes three bugs:

1. Accessors are required at use-site in order to use struct-copy.
This PR removes that requirement since the information is already available in
struct-info. The following program used to fail prior the PR but will now pass.

```

(module a racket
  (provide a)
  (struct a (b)))

(require 'a)
(struct-copy a (a 1) [b 2])
```

2. struct-copy fails if the structure type transformer binding is renamed (racket#1399).
The following program used to fail prior the PR but will now pass.

```

(module struct racket/base
  (provide (struct-out point))
  (struct point (x y) #:transparent))

(require (rename-in 'struct [point point2d]))

(struct-copy point2d (point2d 1 2) [x 3])
```

3. With supertype, it's possible to construct colliding accessors,
causing struct-copy to update an incorrect field. The following program produced
incorrect outputs prior this PR but will now be correct.

```

(module a racket
  (provide a)
  (struct a (b-c) #:transparent))

(require 'a)
(struct a-b a (c) #:transparent)

(struct-copy a-b (a-b 1 2) [b-c #:parent a 10])
;; before the PR: (a-b 1 10), after the PR: (a-b 10 2)
(struct-copy a-b (a-b 1 2) [c 10])
;; before the PR: (a-b 1 10), after the PR: (a-b 1 10)
```

The key idea is that the actual struct name (if the struct is created via `struct`
or `define-struct`) can be extracted from the name of struct descriptor.
The actual field names then can be precisely extracted from accessors.

Note that struct-infos that are created manually by `make-struct-info` didn't work
with struct-copy. This PR didn't attempt to fix that because it requires a significant
change that would not be backward compatible with the current struct info.
sorawee added a commit to sorawee/racket that referenced this issue Apr 23, 2020
This PR fixes four bugs:

1. Accessors are required at use-site in order to use `struct-copy`.
This PR removes that requirement since the information is already available in
struct-info. The following program used to fail prior the PR but will now pass.

```

(module a racket
  (provide a)
  (struct a (b)))

(require 'a)
(struct-copy a (a 1) [b 2])
```

2. `struct-copy` fails if the structure type transformer binding is renamed
(racket#1399). The following program used to fail prior the PR but will now pass.

```

(module struct racket/base
  (provide (struct-out point))
  (struct point (x y) #:transparent))

(require (rename-in 'struct [point point2d]))

(struct-copy point2d (point2d 1 2) [x 3])
```

3. With supertype, it's possible to construct colliding accessors,
causing `struct-copy` to update an incorrect field. The following program
produced incorrect outputs prior this PR but will now be correct.

```

(module a racket
  (provide a)
  (struct a (b-c) #:transparent))

(require 'a)
(struct a-b a (c) #:transparent)

(struct-copy a-b (a-b 1 2) [b-c #:parent a 10])
;; before the PR: (a-b 1 10), after the PR: (a-b 10 2)
(struct-copy a-b (a-b 1 2) [c 10])
;; before the PR: (a-b 1 10), after the PR: (a-b 1 10)
```

4. Similar to 3., prior this commit, it's possible to refer to a bogus field
name when supertype is present. The following program doesn't result in
a syntax error which is wrong. This commit fixes that.

```
(module a racket/base
  (provide (all-defined-out))
  (struct a (b-c) #:transparent))

(require 'a)
(struct a-b a (d) #:transparent)
(struct-copy a-b (a-b 1 2) [c 10])
```

The key idea is that the actual struct name (if the struct is created via
`struct` or `define-struct`) can be extracted from the name of struct predicate.
The actual field names then can be precisely extracted from accessors.

Note that struct-infos that are created manually by `make-struct-info`
didn't work with `struct-copy`. This PR didn't attempt to fix that because
it requires a significant change that would not be backward compatible with
the current struct info.
sorawee added a commit to sorawee/racket that referenced this issue May 7, 2020
This PR fixes four bugs:

1. Accessors are required at use-site in order to use `struct-copy`.
This PR removes that requirement since the information is already available in
struct-info. The following program used to fail prior the PR but will now pass.

```

(module a racket
  (provide a)
  (struct a (b)))

(require 'a)
(struct-copy a (a 1) [b 2])
```

2. `struct-copy` fails if the structure type transformer binding is renamed
(racket#1399). The following program used to fail prior the PR but will now pass.

```

(module struct racket/base
  (provide (struct-out point))
  (struct point (x y) #:transparent))

(require (rename-in 'struct [point point2d]))

(struct-copy point2d (point2d 1 2) [x 3])
```

3. With supertype, it's possible to construct colliding accessors,
causing `struct-copy` to update an incorrect field. The following program
produced incorrect outputs prior this PR but will now be correct.

```

(module a racket
  (provide a)
  (struct a (b-c) #:transparent))

(require 'a)
(struct a-b a (c) #:transparent)

(struct-copy a-b (a-b 1 2) [b-c #:parent a 10])
;; before the PR: (a-b 1 10), after the PR: (a-b 10 2)
(struct-copy a-b (a-b 1 2) [c 10])
;; before the PR: (a-b 1 10), after the PR: (a-b 1 10)
```

4. Similar to 3., prior this commit, it's possible to refer to a bogus field
name when supertype is present. The following program doesn't result in
a syntax error which is wrong. This commit fixes that.

```
(module a racket/base
  (provide (all-defined-out))
  (struct a (b-c) #:transparent))

(require 'a)
(struct a-b a (d) #:transparent)
(struct-copy a-b (a-b 1 2) [c 10])
```

The key idea is that the actual struct name (if the struct is created via
`struct` or `define-struct`) can be extracted from the name of struct predicate.
The actual field names then can be precisely extracted from accessors.

Note that struct-infos that are created manually by `make-struct-info`
didn't work with `struct-copy`. This PR didn't attempt to fix that because
it requires a significant change that would not be backward compatible with
the current struct info.
@samth samth closed this as completed in 52b5f18 May 7, 2020
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

No branches or pull requests

5 participants