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 LT-21088: Yellow Box while parsing words after promoting category #188

Merged
merged 7 commits into from
Oct 30, 2024

Conversation

jtmaxwell3
Copy link
Collaborator

@jtmaxwell3 jtmaxwell3 commented Oct 25, 2024

I fixed LT-21088 by locally defining slots that are in the wrong scope. This keeps XAmple from crashing, but it may or may not parse templates that have slots in the wrong scope correctly. Andy thinks that the right solution is for users to define the scopes of the slots correctly. I'd like to report the problem to the user somehow but I don't see an easy way to do that from ParserCore.


This change is Reviewable

Copy link
Contributor

@JakeOliver28 JakeOliver28 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jtmaxwell3)


Src/LexText/ParserCore/M3ToXAmpleTransformer.cs line 133 at r1 (raw file):

		}

		private void GetUndefinedSlots(XElement element, ISet<string> undefinedSlots)

Could we do this without recursion?

Here's an alternative way of doing it that I believe may work. DescendantsAndSelf() gets a flat structure that can be simply iterated over. Unsure if the order is fine without recursion based on your last comment in this method.

Code snippet:

private void GetUndefinedSlots(XElement element, ISet<string> undefinedSlots)
{
	// Iterate over all descendant elements in a flat structure.
	foreach (XElement element in rootElement.DescendantsAndSelf())
	{
		// Record slots encountered.
		if (element.Name == "PrefixSlots" || element.Name == "SuffixSlots")
		{
			undefinedSlots.Add((string)element.Attribute("dst"));
		}

		// Check and remove slots that are defined at this level.
		if (element.Name == "AffixSlots")
		{
			foreach (XElement slot in element.Elements())
			{
				undefinedSlots.Remove((string)slot.Attribute("Id"));
			}
		}
	}
}

@jtmaxwell3
Copy link
Collaborator Author

I think that the flat version may get the wrong result because AffixSlots has to be processed after PrefixSlots and SuffixSlots. I would prefer to keep the recursive version because it is more obvious whether this constraint is preserved.

Copy link
Contributor

@JakeOliver28 JakeOliver28 left a comment

Choose a reason for hiding this comment

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

Could we do two passes then? The recursive code feels less readable/easily understandable to me.

private void GetUndefinedSlots(XElement rootElement, ISet<string> undefinedSlots)
{
	var allElements = rootElement.DescendantsAndSelf();	

	// First pass: Add slots encountered in PrefixSlots or SuffixSlots
	foreach (XElement element in allElements)
	{
		if (element.Name == "PrefixSlots" || element.Name == "SuffixSlots")
		{
			undefinedSlots.Add((string)element.Attribute("dst"));
		}
	}

	// Second pass: Remove any slots that are defined in AffixSlots elements
	foreach (XElement element in allElements)
	{
		if (element.Name == "AffixSlots")
		{
			foreach (XElement slot in element.Elements())
			{
				undefinedSlots.Remove((string)slot.Attribute("Id"));
			}
		}
	}
}

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jtmaxwell3)

@JakeOliver28 JakeOliver28 enabled auto-merge (squash) October 29, 2024 19:38
Copy link
Contributor

@JakeOliver28 JakeOliver28 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jtmaxwell3)


Src/LexText/ParserCore/M3ToXAmpleTransformer.cs line 118 at r1 (raw file):

				return;
			// Add undefined slots to AffixSlots.
			foreach (XElement elem in templateElem.Elements())

We can get rid of the outer foreach loop by grabbing the first element with "AffixSlots" as its name.

We could also get rid of the inner foreach loop with LINQ, although that change is arguably beneficial.

Code snippet:

	XElement affixSlotsElem = templateElem.Element("AffixSlots");
    if (affixSlotsElem == null)
        return;

    affixSlotsElem.Add(CreateAffixSlotElements(undefinedSlots));
    
    
   
// Separate helper method for readability
private IEnumerable<XElement> CreateAffixSlotElements(IEnumerable<string> slotIds)
{
    return slotIds.Select(slotId => new XElement("MoInflAffixSlot", new XAttribute("Id", slotId)));
}

@jtmaxwell3
Copy link
Collaborator Author

Two flat passes gets the wrong result. We are trying to determine whether there is a variable that is used but has no definition in scope. So we have to use recursion to compute scope.

I don't understand your second suggestion, so I can't verify if it is correct. I find LINQ code hard to read.

I don't see what is wrong with my original code.

Copy link
Contributor

@JakeOliver28 JakeOliver28 left a comment

Choose a reason for hiding this comment

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

Sounds good!

Just wanting to avoid nested foreach loops & an if statement if possible. For my second suggestion, we can leave out the LINQ and still remove the outer foreach loop.

XElement affixSlotsElem = templateElem.Element("AffixSlots");
if (affixSlotsElem == null)
   return;

foreach (string slotId in undefinedSlots)
{
	XElement slot = new XElement("MoInflAffixSlot");
	slot.SetAttributeValue("Id", slotId);
	affixSlotsElem.Add(slot);
}

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jtmaxwell3)

@jtmaxwell3
Copy link
Collaborator Author

I removed the extra for loop and made the documentation clearer.

Copy link
Contributor

@JakeOliver28 JakeOliver28 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jtmaxwell3)

@JakeOliver28 JakeOliver28 merged commit 65823a5 into release/9.1 Oct 30, 2024
4 of 5 checks passed
@JakeOliver28 JakeOliver28 deleted the LT-21088 branch October 30, 2024 18:15
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